By chance, two things have prevented the autoupgrade path being
exercised much so far:
* LLParser setting the debug-info mode to "old" on seeing intrinsics,
* The test in AutoUpgrade.cpp wanting to upgrade into a "new" debug-info
block.
In practice, this appears to mean this code path hasn't seen the various
invalid inputs that can come its way. This commit does a number of
things:
* Tolerates the various illegal inputs that can be written with
debug-intrinsics, and that must be tolerated until the Verifier runs,
* Printing illegal/null DbgRecord fields must succeed,
* Verifier errors need to localise the function/block where the error
is,
* Tests that now see debug records will print debug-record errors,
Plus a few new tests for other intrinsic-to-debug-record failures modes
I found. There are also two edge cases:
* Some of the unit tests switch back and forth between intrinsic and
record modes at will; I've deleted coverage and some assertions to
tolerate this as intrinsic support is now Gone (TM),
* In sroa-extract-bits.ll, the order of debug records flips. This is
because the autoupgrader upgrades in the opposite order to the basic
block conversion routines... which doesn't change the record order, but
_does_ change the use list order in Metadata! This should (TM) have no
consequence to the correctness of LLVM, but will change the order of
various records and the order of DWARF record output too.
I tried to reduce this patch to a smaller collection of changes, but
they're all intertwined, sorry.
These date back to when the non-intrinsic format of variable locations
was still being tested and was behind a compile-time flag, so not all
builds / bots would correctly run them. The solution at the time, to get
at least some test coverage, was to have tests opt-in to non-intrinsic
debug-info if it was built into LLVM.
Nowadays, non-intrinsic format is the default and has been on for more
than a year, there's no need for this flag to exist.
(I've downgraded the flag from "try" to explicitly requesting
non-intrinsic format in some places, so that we can deal with tests that
are explicitly about non-intrinsic format in their own commit).
Fixes issue #61981 by adjusting variable location offsets (in the DIExpression)
when splitting allocas.
Patch [4/4] to fix structured bindings in SROA.
NOTE: There's still a bug in mem2reg which generates incorrect locations in some
situations: if the variable fragment has an offset into the new (split) alloca,
mem2reg will fail to convert that into a bit shift (the location contains a
garbage offset). That's not addressed here.
insertNewDbgInst - Now takes the address-expression and FragmentInfo as
separate parameters because unlike dbg_declares dbg_assigns want those to go
to different places. dbg_assign records put the variable fragment info in the
value expression only (whereas dbg_declare has only one expression so puts it
there - ideally this information wouldn't live in DIExpression, but that's
another issue).
MigrateOne - Modified to correctly compute the necessary offsets and fragment
adjustments. The previous implementation produced bogus locations for variables
with non-zero offsets. The changes replace most of the body of this lambda, so
it might be easier to review in a split-diff view and focus on the change as a
whole than to compare it to the old implementation.
This uses calculateFragmentIntersect and extractLeadingOffset added in previous
patches in this series, and createOrReplaceFragment described below.
createOrReplaceFragment - Similar to DIExpression::createFragmentExpression
except for 3 important distinctions:
1. The new fragment isn't relative to an existing fragment.
2. There are no checks on the the operation types because it is assumed
the location this expression is computing is not implicit (i.e., it's
always safe to create a fragment because arithmetic operations apply
to the address computation, not to an implicit value computation).
3. Existing extract_bits are modified independetly of fragment changes
using \p BitExtractOffset. A change to the fragment offset or size
may affect a bit extract. But a bit extract offset can change
independently of the fragment dimensions.
Returns the new expression, or nullptr if one couldn't be created. Ideally
this is only used to signal that a bit-extract has become zero-sized (and thus
the new debug record has no size and can be dropped), however, it fails for
other reasons too - see the FIXME below.
FIXME: To keep the scope of this change focused on non-bitfield structured
bindings the function bails in situations that
DIExpression::createFragmentExpression fails. E.g. when fragment and bit
extract sizes differ. These limitations can be removed in the future.
If the instruction is marked for deletion, better to drop all its
operands and mark them for deletion too (if allowed). It allows to have
more vectorizable patterns and generate less useless extractelement
instructions.
Reviewers: RKSimon
Reviewed By: RKSimon
Pull Request: https://github.com/llvm/llvm-project/pull/97409
This patch makes the final major change of the RemoveDIs project, changing the
default IR output from debug intrinsics to debug records. This is expected to
break a large number of tests: every single one that tests for uses or
declarations of debug intrinsics and does not explicitly disable writing
records.
If this patch has broken your downstream tests (or upstream tests on a
configuration I wasn't able to run):
1. If you need to immediately unblock a build, pass
`--write-experimental-debuginfo=false` to LLVM's option processing for all
failing tests (remember to use `-mllvm` for clang/flang to forward arguments to
LLVM).
2. For most test failures, the changes are trivial and mechanical, enough that
they can be done by script; see the migration guide for a guide on how to do
this: https://llvm.org/docs/RemoveDIsDebugInfo.html#test-updates
3. If any tests fail for reasons other than FileCheck check lines that need
updating, such as assertion failures, that is most likely a real bug with this
patch and should be reported as such.
For more information, see the recent PSA:
https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578
llvm.dbg.assign intrinsics have 2 {value, expression} pairs; fix hwasan to
update the second expression.
Fixes#76545. This is #78606 rebased and with the addition of DPValue handling.
Note the addition of --try-experimental-debuginfo-iterators in the tests and
some shuffling of code in MemoryTaggingSupport.cpp.
With this, I get a clean test suite running under RemoveDIs, the
non-intrinsic representation of debug-info, including under asan. We've
previously established that we generate identical binaries for some
large projects, so this i just edge-case cleanup. The changes:
* CodeGenPrepare fixups need to apply to dbg.assigns as well as
dbg.values (a dbg.assign is a dbg.value).
* Pin a test for constant-deletion to intrinsic debug-info: this very
rare scenario uses a different kill-location sigil in dbg.value mode to
RemoveDIs mode, which generates spurious test differences.
* Suppress a memory leak in a unit test: the code for dealing with
trailing debug-info in a block is necessarily fiddly, leading to this
leak when testing it. Developer-facing interfaces for moving
instructions around always deal with this behind the scenes.
* SROA, when replacing some vector-loads, needs to insert the
replacement loads ahead of any debug-info records so that their values
remain dominated by a definition. Set the head-bit indicating our
insertion should come before debug-info.
This is the final patch for DPVAssign support, implementing the actual
creation of DPVAssigns and allowing them to be converted along with
dbg.values and dbg.declares. Numerous tests landed in previous patches
will no longer be rotten after this patch lands (previously they would
trivially pass due to DPVAssigns not actually being used), and a further
batch of tests have been added here that require the changes in this
patch before they pass.
This patch trivially updates various opt passes to handle DPVAssigns. In
all cases, this means some combination of generifying existing code to
handle DPValues and DbgAssignIntrinsics, iterating over DPValues where
previously we did not, or duplicating code for DbgAssignIntrinsics to
the equivalent DPValue function (in inlining and salvageDebugInfo).
As part of a recent patch landing, some tests that are not yet ready
to support RemoveDIs were turned on; this patch disables those tests
in RemoveDIs mode.
Fixes buildbot failure:
https://lab.llvm.org/buildbot/#/builders/275/builds/3640
SROA needs to update llvm.dbg.assign intrinsics when it migrates debug
info in response to alloca splitting; this patch updates the debug info
migration code to handle DPVAssigns as well, making use of generic code
to avoid duplication as much as possible.
This patch fixes commit 89aa3355, which added tests for
the removal of redundant DPVAssigns; unlike other cases where
adding tests for DPVAssigns before they are enabled is harmless,
these tests require them to be enabled, so must be deleted until
we enable them.
Fixes failures on llvm-new-debug-iterators buildbot:
https://lab.llvm.org/buildbot/#/builders/275/builds/3581
DPValues are already supported by most of the utilities that remove
redundant debug info after certain passes; the exception to this is
`removeUndefDbgAssignsFromEntryBlock`, which applies only to
llvm.dbg.assigns which were previously unimplemented for DPValues. Now
that DPVAssigns exist, we have to support removing redundant instances
in the same way, which this patch implements.
Temporarily fix for issue #76545
Hwasan does not attach tags to @llvm.dbg.assign. It's not clear if we
can attach tags to @llvm.dbg.assign.
For now we just disable the path replacing llvm.dbg.declare with
llvm.dbg.assign.
It may reduce the quality of interactive debugging with HWASAN, but
usually it's
a smaller priority for sanitizers than the quality if reports.
Explicitly inserting undef is overly defensive. Any values computed
nside the loop that are referenced by dbg.values should naturally
become undef when the loop is deleted, and all other values that
are loop invariant must be preserved.
Reviewed By: scott.linder
Differential Revision: https://reviews.llvm.org/D153539
Do not convert dbg.declares to dbg.assigns for variables backed by scalable
vector allocas as this isn't yet supported.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D149959
`shortenAssignment` inserts dbg.assigns with fragments describing the dead part
of a shortened store after each dbg.assign linked to the store.
Without this patch it doesn't take into account that the dead part of a
shortened store may be outside the bounds of a variable of a linked
dbg.assign. It also doesn't correctly account for a non-zero offset in the
address modifying `DIExpression` of the dbg.assign (which is possible for
fragments now even though whole variables currently cannot have a non-zero
offset in their alloca).
Fix this by moving the dead slice into variable-space and performing an
intersect of that adjusted slice with the existing fragment.
This fixes a verifier error reported when building fuchsia with assignment
tracking enabled:
https://ci.chromium.org/ui/p/fuchsia/builders/ci/
clang_toolchain.ci.core.x64-release/b8784000953022145169/overview
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D148536
Debug intrinsics sometimes end up with empty metadata location operands. The
debug intrinsic interfaces return nullptr when retrieving location operand in
this case.
Skip empty-metadata dbg.declares to avoid dereferencing the nullptr. This
doesn't affect the final debug info in any way.
Reviewed By: jryans
Differential Revision: https://reviews.llvm.org/D148204
VLA backed variables currently trip an assertion in SROA with D146987 (enabling
assignment tracking). Disable assignment tracking for VLA variables until that
can be investigated.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D148140
The assertion exists to ensure all variables passed into `trackAssignments` end
up with dbg.assigns associated with their backing allocas. The assertion
compared the passed-in and tracked variables using `DebugVariable`, which
includes the fragment as part of the variable identity.
It is possible for the backing alloca to be smaller than a variable (see test
case). In this case the input variable `(Var X, no fragment, no InlinedAt)`
isn't equal to the dbg.assign variable `(Var X, some fragment, no
InlinedAt)`. To cover this case the assertion now ignores fragments through the
use of `DebugVariableAggregate`.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D148100
In D147777 emitDbgAssign was fixed to discard assignments which touched any
bits outside the bounds of a variable. This patch changes emitDbgAssign to
discard assignments which touch bits only outside the variable bounds, and
creates a truncated fragment expression for stores partially overlapping the
variable. This is necessary because the alloca is interpreted as a store (of
undef), meaning without this patch emitDbgAssign would discard the inital
dbg.assign for a variable that is smaller than the alloca.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D148018
Some dbg.assigns using poison become un-poisoned in SROA. The reason this
happens at all is because dbg.assigns linked to memory intrinsics use poison to
indicate they can't describe the stored value, but the value becomes available
after some optimisations. This needs reworking eventually, but for now we need
to ensure that when it does occur we don't create invalid expressions.
D147312 prevented this occuring when the dbg.assign uses DIArgLists, but that
wasn't a complete fix. We also need to ensure we avoid un-poisoning when the
existing expression uses more than one location operand (DW_OP_arg, n).
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D148020
Prior to this patch the trackAssignments function would attribute all stores to
an alloca to all variables linked to the alloca. This is wrong in the case
where the alloca contains variables which are smaller than the alloca, and
caused erroneous fragment information to be generated.
Now stores outside the variable bounds are discarded, and we check whether a
fragment is needed based on whether the store covers the entire variable as
opposed to whether it covers the entire alloca (except for variables of unknown
size).
Note that trackAssignments doesn't yet understand whole variables sitting at
anything other than offset 0 in an alloca - those variables are still tracked
using dbg.declares.
Fixes https://lab.llvm.org/buildbot/#/builders/70/builds/36007
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D147777
Correctly handle the case of splitting an alloca which backs contiguous
distinct variables, where a slice's size equals the size of a backed variable.
We need to ensure that we don't generate fragments expressions with fragments
of the same size as the variable as this is a verifier error.
Prior to this patch a fragment expression would be created in this
situation. e.g. splitting an alloca i64 with two adjacent 32-bit variables into
two 32-bit allocas, the new dbg.assign expressions would contain
(DW_OP_LLVM_fragment, 0, 32) and (DW_OP_LLVM_fragment, 32, 32) even though
those fragments cover each variable entirely.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D147696
createFragmentExpression will fail if it determines that the expression cannot
be split over fragments. Handle this case in SROA. Similarly to D147312 this
should be a rare occurrence as the `dbg.assign` will usually reference the
`Value` being stored without modifying it with a `DIExpression`.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D147431
If the to-be-split dbg.assign has a `DIArgList` and a new `Value` has been
requested then use a kill-location for the new dbg.assign. We can't simply
replace the value component (a `DIArgList`) with the new `Value` as that would
leave the `DIExpression` in an invalid state (`DW_OP_LLVM_arg` operands with no
arglist).
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D147312
Assignment tracking doesn't add value when optimisations are disabled - don't
apply it to functions marked optnone.
Reviewed By: jryans
Differential Revision: https://reviews.llvm.org/D147129
updateForDeletedStore updates the assignment tracking debug info for a store
that is about to be deleted by mem2reg. For each variable backed by the target
alloca, if a dbg.assign exists it is kept (well - it's downgraded to a
dbg.value). A dbg.value is inserted if there's not a linked dbg.assign for a
variable which is backed by the target alloca. This patch fixes a bug whereby a
store with a linked dbg.assign that describes a fragment different to the one
linked to the alloca was not counted for the variable, leading to both keeping
the dbg.assign (downgrading it) and inserting a new dbg.value.
Reviewed By: StephenTozer
Differential Revision: https://reviews.llvm.org/D146299
For fully promoted variables dbg.assigns and dbg.values convey the same
information and can be used interchangeably. This patch converts dbg.assigns to
dbg.values for variables promoted by mem2reg. This reduces resource usage by
reducing the amount of unnecessary function local metadata. The compile time
tracker reports that CTMark projects build with LTO-O3-g with 0.4% fewer
instructions retired and peak memory usage is reduced by 2.2%.
Reviewed By: jryans
Differential Revision: https://reviews.llvm.org/D145511
Allow the value-component of a dbg.assign to be salvaged using a variadic
DIExpression.
Reviewed By: StephenTozer
Differential Revision: https://reviews.llvm.org/D145915
The module flag "debug-info-assignment-tracking" is set by the
declare-to-assign pass to indicate that assignment tracking is enabled. This
patch changes declare-to-assign to only set the flag if it makes a
modification. This lets the compiler avoid doing extra work for no benefit,
such as is currently the case if assignment tracking is requested for a build
with line tables only (-gmlt) or a build without debug info.
Reviewed By: scott.linder
Differential Revision: https://reviews.llvm.org/D144481
Enabling assignment tracking without this patch, a significant amount of
additional compiler run time comes from the RemoveRedundantDbgInstrs call in
InstCombine. This patch reduces compiler run time by choosing better places to
call RemoveRedundantDbgInstrs.
In non-assignment-tracking builds, RemoveRedundantDbgInstrs is called by
InstCombine if LowerDbgDeclare makes a change (i.e. it is _sometimes_
called). In assignment tracking builds LowerDbgDeclare doesn't do anything. We
still need to clean up redundant intrinsics to avoid a large performance hit
due to the number of instructions, so the current approach is to have
InstCombine _always_ call RemoveRedundantDbgInstrs.
Instrumenting the compiler to run RemoveRedundantDbgInstrs after every pass and
dump the numbers and building CTMark/tramp3d-v4 indicates that SROA and
LoopVectorize give us a bigger bang (number removed) for buck (times pass is
run).
The compile time tracker reports that this patch reduces the number of
instructions retired building CTMark projects by an average of 1.1%.
Reviewed By: scott.linder
Differential Revision: https://reviews.llvm.org/D144483
Without this patch, migrateDebugInfo doesn't understand how to handle existing
fragments that are smaller than the to-be-split store. This can occur
if. e.g. a vector store (1 dbg.assign) is split (many dbg.assigns - 1 fragment
for each scalar) and later those stores are re-vectorized (many dbg.assigns),
and then SROA runs on that.
The approach taken in this patch is to drop intrinsics with fragments outside
of the slice.
For example, starting with:
store <2 x float> %v, ptr %dest !DIAssignID !1
call void @llvm.dbg.assign(..., DIExpression(DW_OP_LLVM_fragment, 0, 32), !1, ...)
call void @llvm.dbg.assign(..., DIExpression(DW_OP_LLVM_fragment, 32, 32), !1, ...)
When visiting the slice of bits 0 to 31 we get:
store float %v.extract.0, ptr %dest !DIAssignID !2
call void @llvm.dbg.assign(..., DIExpression(DW_OP_LLVM_fragment, 0, 32), !2, ...)
The other dbg.assign associated with the currently-split store is dropped for
this split part. And visiting bits 32 to 63 we get the following:
store float %v.extract.1, ptr %adjusted.dest !DIAssignID !3
call void @llvm.dbg.assign(..., DIExpression(DW_OP_LLVM_fragment, 32, 32), !3, ...)
I've added two tests that cover this case.
Implementing this meant re-writing the fragment-calculation part of
migrateDebugInfo to work with the absolute offset of the new slice in terms of
the base alloca (instead of the offset of the slice into the new alloca), the
fragment (if any) of the variable associated with the base alloca, and the
fragment associated with the split store. Because we need the offset into the
base alloca for the variables being split, some careful wiring is required for
memory intrinsics due to the fact that memory intrinsics can be split when
either the source or dest allocas are split. In the case where the source
alloca drives the splitting, we need to be careful to pass migrateDebugInfo the
information in relation to the dest alloca.
Reviewed By: StephenTozer
Differential Revision: https://reviews.llvm.org/D143146
AggLoadStoreRewriter splits aggregate loads and stores into scalars (before the
alloca is split up). The new stores and debug intrinsics are already wired up
correctly - we just need to also delete the dbg.assign that is linked to the
split to-be-deleted store too.
Reviewed By: jmorse
Differential Revision: https://reviews.llvm.org/D142882
The intrinsics don't add anything to the tests and the tests are easier to
debug without the additional noise.
Some SSA value names have changed as a result of no longer visiting the
intrinsics.
Reviewed By: jryans
Differential Revision: https://reviews.llvm.org/D143141
Remove LLVM flag -experimental-assignment-tracking. Assignment tracking is
still enabled from Clang with the command line -Xclang
-fexperimental-assignment-tracking which tells Clang to ask LLVM to run the
pass declare-to-assign. That pass converts conventional debug intrinsics to
assignment tracking metadata. With this patch it now also sets a module flag
debug-info-assignment-tracking with the value `i1 true` (using the flag conflict
rule `Max` since enabling assignment tracking on IR that contains only
conventional debug intrinsics should cause no issues).
Update the docs and tests too.
Reviewed By: CarlosAlbertoEnciso
Differential Revision: https://reviews.llvm.org/D142027
