Since some places, like SimplifyCFG, work with 64-bit weights, we supply
an API in ProfDataUtils to extract the weights accordingly.
We change the API slightly to disambiguate the 64-bit version from the
32-bit version.
When speculating a store based on a preceding load/store, we need
to ensure that the speculated store does not have a higher
alignment (which might only be guaranteed by the branch condition).
There are various ways in which this could be strengthened (we
could get or enforce the alignment), but for now just do the
simple check against the preceding load/store.
Fixes https://github.com/llvm/llvm-project/issues/89672.
The large case index out of scope is dead code, but it is still be
created for TableContents in SwitchLookupTable::SwitchLookupTable,
so make sure the table size after growing should not get smaller.
Fix https://github.com/llvm/llvm-project/issues/88607
This is the major rename patch that prior patches have built towards.
The DPValue class is being renamed to DbgVariableRecord, which reflects
the updated terminology for the "final" implementation of the RemoveDI
feature. This is a pure string substitution + clang-format patch. The
only manual component of this patch was determining where to perform
these string substitutions: `DPValue` and `DPV` are almost exclusively
used for DbgRecords, *except* for:
- llvm/lib/target, where 'DP' is used to mean double-precision, and so
appears as part of .td files and in variable names. NB: There is a
single existing use of `DPValue` here that refers to debug info, which
I've manually updated.
- llvm/tools/gold, where 'LDPV' is used as a prefix for symbol
visibility enums.
Outside of these places, I've applied several basic string
substitutions, with the intent that they only affect DbgRecord-related
identifiers; I've checked them as I went through to verify this, with
reasonable confidence that there are no unintended changes that slipped
through the cracks. The substitutions applied are all case-sensitive,
and are applied in the order shown:
```
DPValue -> DbgVariableRecord
DPVal -> DbgVarRec
DPV -> DVR
```
Following the previous rename patches, it should be the case that there
are no instances of any of these strings that are meant to refer to the
general case of DbgRecords, or anything other than the DPValue class.
The idea behind this patch is therefore that pure string substitution is
correct in all cases as long as these assumptions hold.
This patch changes DPValue::filter to be a non-member method
filterDbgVars. There are two reasons for this: firstly, the name of
DPValue is about to change to DbgVariableRecord, which will result in
every `for` loop that uses DPValue::filter to require a line break. This
is a small thing, but it makes the rename patch more difficult to
review, and is just generally more awkward for what is a fairly common
loop. Secondly, the intent is to later break up the DPValue class into
subclasses, at which point it would be better to have a non-member
function that allows template arguments for the cases we want to filter
with greater specificity.
This patch continues the ongoing rename work, replacing DPValue with
DbgRecord in comments and the names of variables, both members and
fn-local. This is the most labour-intensive part of the rename, as it is
where the most decisions have to be made about whether a given comment
or variable is referring to DPValues (equivalent to debug variable
intrinsics) or DbgRecords (a catch-all for all debug intrinsics); these
decisions are not individually difficult, but comprise a fairly large
amount of text to review.
This patch still largely performs basic string substitutions followed by
clang-format; there are almost* no places where, for example, a comment
has been expanded or modified to reflect the semantic difference between
DPValues and DbgRecords. I don't believe such a change is generally
necessary in LLVM, but it may be useful in the docs, and so I'll be
submitting docs changes as a separate patch.
*In a few places, `dbg.values` was replaced with `debug intrinsics`.
As part of the effort to rename the DbgRecord classes, this patch
renames the widely-used functions that operate on DbgRecords but refer
to DbgValues or DPValues in their names to refer to DbgRecords instead;
all such functions are defined in one of `BasicBlock.h`,
`Instruction.h`, and `DebugProgramInstruction.h`.
This patch explicitly does not change the names of any comments or
variables, except for where they use the exact name of one of the
renamed functions. The reason for this is reviewability; this patch can
be trivially examined to determine that the only changes are direct
string substitutions and any results from clang-format responding to the
changed line lengths. Future patches will cover renaming variables and
comments, and then renaming the classes themselves.
I'd reverted this in 6c7805d5d1 after a bad stage. Original commit
messsage follows:
[NFC][RemoveDIs] Bulk update utilities to insert with iterators
As part of the RemoveDIs project we need LLVM to insert instructions using
iterators wherever possible, so that the iterators can carry a bit of
debug-info. This commit implements some of that by updating the contents of
llvm/lib/Transforms/Utils to always use iterator-versions of instruction
constructors.
There are two general flavours of update:
* Almost all call-sites just call getIterator on an instruction
* Several make use of an existing iterator (scenarios where the code is
actually significant for debug-info)
The underlying logic is that any call to getFirstInsertionPt or similar
APIs that identify the start of a block need to have that iterator passed
directly to the insertion function, without being converted to a bare
Instruction pointer along the way.
I've also switched DemotePHIToStack to take an optional iterator: it needs
to take an iterator, and having a no-insert-location behaviour appears to
be important. The constructors for ICmpInst and FCmpInst have been updated
too. They're the only instructions that take block _references_ rather than
pointers for certain calls, and a future patch is going to make use of
default-null block insertion locations.
All of this should be NFC.
As part of the RemoveDIs project we need LLVM to insert instructions using
iterators wherever possible, so that the iterators can carry a bit of
debug-info. This commit implements some of that by updating the contents of
llvm/lib/Transforms/Utils to always use iterator-versions of instruction
constructors.
There are two general flavours of update:
* Almost all call-sites just call getIterator on an instruction
* Several make use of an existing iterator (scenarios where the code is
actually significant for debug-info)
The underlying logic is that any call to getFirstInsertionPt or similar
APIs that identify the start of a block need to have that iterator passed
directly to the insertion function, without being converted to a bare
Instruction pointer along the way.
I've also switched DemotePHIToStack to take an optional iterator: it needs
to take an iterator, and having a no-insert-location behaviour appears to
be important. The constructors for ICmpInst and FCmpInst have been updated
too. They're the only instructions that take block _references_ rather than
pointers for certain calls, and a future patch is going to make use of
default-null block insertion locations.
All of this should be NFC.
llvm.dbg.labels are deleted in SpeculativelyExecuteBB so DPLabels should
be too.
Modify existing test to check this (NB I couldn't find a dedicated
debug-info test that checks this behaviour).
Patch 1 of 3 to add llvm.dbg.label support to the RemoveDIs project. The
patch stack adds a new base class
-> 1. Add DbgRecord base class for DPValue and the not-yet-added
DPLabel class.
2. Add the DPLabel class.
3. Enable dbg.label conversion and add support to passes.
Patches 1 and 2 are NFC.
In the near future we also will rename DPValue to DbgVariableRecord and
DPLabel to DbgLabelRecord, at which point we'll overhaul the function
names too. The name DPLabel keeps things consistent for now.
Follow up to #79476 - that patch added a call to hoistLockstepIdenticalDPValues
which hoists identical DPValues in lockstep, matching dbg intrinsic hoisting
behaviour. The code deleted in this patch, which unconditionally hoists
DPValues, should have been deleted in that patch.
Update test with --try-experimental-debuginfo-iterators to check the behaviour.
Follow up to #79476 - that change introduces a call to
hoistLockstepIdenticalDPValues.
Hoist DPValues attached to each instruction being considered for hoisting if
they are identical in lock-step. This includes the final instructions which
are considered but not hoisted, because the corresponding dbg.values would
appear before those instruction and thus hoisted if identical.
Identical debug records hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue.ll
Non-identical debug records not hoisted:
llvm/test/Transforms/SimplifyCFG/X86/pr39187-g.ll
Debug records attached to first not-hoisted instructions are hoisted:
llvm/test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll
This relands commit f890f010f6a70addbd885acd0c8d1b9578b6246f.
The result value of `getelementptr inbounds (TY, null, not zero)` is a poison value.
We can think of it as undefined behavior.
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).
https://github.com/llvm/llvm-project/pull/76669 taught SimplifyCFG to
handle switches when `default` has only one case. When the `switch`'s
condition is wider than 64 bit, the current implementation can calculate
the wrong default value. This PR skips cases where the condition is too
wide.
The specialisation will not be valid when ConstantInt gains native
support for vector types.
This is largely a mechanical change but with extra attention paid to constant
folding, InstCombineVectorOps.cpp, LoopFlatten.cpp and Verifier.cpp to
remove the need to call `getIntegerType()`.
Co-authored-by: Nikita Popov <github@npopov.com>
This patch adds support for CloneBasicBlock duplicating the DPValues
attached to instructions, and adds facilities to remap them into their new
context. The plumbing to achieve this is fairly straightforwards and
mechanical.
I've also added illustrative uses to LoopUnrollRuntime, SimpleLoopUnswitch
and SimplifyCFG. The former only updates for the epilogue right now so I've
added CHECK lines just for the end of an unrolled loop (further updates
coming later). SimpleLoopUnswitch had no debug-info tests so I've added a
new one. The two modified parts of SimplifyCFG are covered by the two
modified SimplifyCFG tests.
These are scenarios where we have to do extra cloning for copying of
DPValues because they're no longer instructions, and remap them too.
The code in the CloneInstructionsIntoPredec... function modified by this
patch has a long history that dates back to 2011, see d715ec82b4ad12c59.
There, when folding branches, all dbg.value intrinsics seen when folding
would be saved and then re-inserted at the end of whatever was folded. Over
the last 12 years this behaviour has been preserved.
However, IMO it's bad behaviour. If we have:
inst1
dbg.value1
inst2
dbg.value2
And we fold that sequence into a different block, then we would want the
instructions and variable assignments to appear in the same order. However
because of this old behaviour, the dbg.values are sunk, and we get:
inst1
inst2
dbg.value1
dbg.value2
This clustering of dbg.values can make assignments to the same variable
invisible, as well as reducing the coverage of other assignments.
This patch relaxes the CloneInstructions... function and allows it to clone
and update dbg.values in-place, causing them to appear in the original
order in the destination block. I've added some extra dbg.values to the
updated test: without the changes to the pass, the dbg.values sink into a
blob ahead of the select. The RemoveDIs code can't cope with this right now
so I've removed the "--try..." flag, restored in a commit to land in a
couple of hours.
(Metadata changes to make the LLVM-IR parser not drop the debug-info for it
being out of date. The RemoveDIs related RUN line has been removed because
it was spuriously passing due to the debug-info being dropped).
In present-day debug-info, when you delete all instructions, you delete
all their debug-info with it because debug-info is stored in
instructions. With debug-info stored in DPValue objects however,
deleting instructions causes DPValue objects to clump together into a
large blob of debug-info that hangs around in the block, as nothing has
explicitly deleted it.
To restore this behaviour, scatter calls to dropDbgValues around in
places that used to delete chunks of dbg.values, for example during
stripDebugInfo and in the code that deletes everything after an
Unreachable instruction. DCE is another example.
The tests with --try... added to them are new scenarios where we can now
correctly replicate the "normal" debug-info behaviour. Alas, there's no
explicit test for the opt -strip-debug option though (in dbg.value mode
or DPValue mode).
Optimization reduces the range for switches whose cases are positive powers
of two by replacing each case with count_trailing_zero(case).
Resolves#70756
AMDGPU target has faced the situation which can be illustrated with the
following testcase:
define void @dont_merge_cbranches(i32 %V) {
%divergent_cond = icmp ne i32 %V, 0
%uniform_cond = call i1 @uniform_result(i1 %divergent_cond)
br i1 %uniform_cond, label %bb2, label %exit, !prof !0
bb2:
br i1 %divergent_cond, label %bb3, label %exit
bb3:
call void @bar( )
br label %exit
exit:
ret void
}
!0 = !{!"branch_weights", i32 1, i32 100000}
SimplifyCFG merges branches on %uniform_cond and %divergent_cond which is undesirable because the first branch to bb2 is taken extremely rare and the second branch is expensive. The merged branch becomes as expensive as the second.
This patch prevents such merging if the branch to the second branch is unlikely to happen.
Fix the crash for the last land PR70542.
Note:
For '%add = add nuw i32 %x, 1', we can only infer the LowerBound is 1,
but the UpperBound is wrapped to 0 in computeConstantRange.
so we can't assume the UpperBound is valid bound when its value is 0.
Fix https://github.com/llvm/llvm-project/issues/71329.
Reviewed By: zmodem, nikic
When the small mask value little than 64, we can eliminate the checking
for upper limit of the range by enlarge the lookup table size to the maximum
index value. (Then the final table size grows to the next pow2 value)
```
bool f(unsigned x) {
switch (x % 8) {
case 0: return 1;
case 1: return 0;
case 2: return 0;
case 3: return 1;
case 4: return 1;
case 5: return 0;
case 6: return 1;
// This would remove the range check: case 7: return 0;
}
return 0;
}
```
Use WouldFitInRegister instead of fitsInLegalInteger to support
more result type beside bool.
Fixes https://github.com/llvm/llvm-project/issues/65120
Reviewed By: zmodem, nikic, RKSimon
C++20 comes with std::erase to erase a value from std::vector. This
patch renames llvm::erase_value to llvm::erase for consistency with
C++20.
We could make llvm::erase more similar to std::erase by having it
return the number of elements removed, but I'm not doing that for now
because nobody seems to care about that in our code base.
Since there are only 50 occurrences of erase_value in our code base,
this patch replaces all of them with llvm::erase and deprecates
llvm::erase_value.
This reverts commit 96ea48ff5dcba46af350f5300eafd7f7394ba606.
The change may cause Verifier.cpp error
"musttail call must precede a ret with an optional bitcast"
As per the stack of patches this is attached to, allow users of
BasicBlock::splitBasicBlock to provide an iterator for a position, instead
of just an instruction pointer. This is to fit with my proposal for how to
get rid of debug intrinsics [0]. There are other call-sites that would need
to change, but this is sufficient for a stage2clang self host and some
other C++ projects to build identical binaries, in the context of the whole
remove-DIs project.
[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939
Differential Revision: https://reviews.llvm.org/D152545
Continuing the patch series to get rid of debug intrinsics [0], instruction
insertion needs to be done with iterators rather than instruction pointers,
so that we can communicate information in the iterator class. This patch
adds an iterator-taking insertBefore method and converts various call sites
to take iterators. These are all sites where such debug-info needs to be
preserved so that a stage2 clang can be built identically; it's likely that
many more will need to be changed in the future.
At this stage, this is just changing the spelling of a few operations,
which will eventually become signifiant once the debug-info bearing
iterator is used.
[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939
Differential Revision: https://reviews.llvm.org/D152537
As outlined in my proposal of how to get rid of debug intrinsics, this
patch adds a moveBefore method that signals the caller /intends/ the order
of moved instructions is to stay the same. This semantic difference has an
effect on debug-info, as it signals whether debug-info needs to move with
instructions or not.
The patch just replaces a few calls to moveBefore with calls to
moveBeforePreserving -- and the latter just calls the former, so it's all
NFC right now. A future patch will add an implementation of
moveBeforePreserving that takes action to correctly preserve debug-info,
but that's tightly coupled with our non-instruction debug-info
representation that's still being reviewed.
[0] https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939
Differential Revision: https://reviews.llvm.org/D156369
isLegalToHoistInto() currently return true for callbr instructions.
That means that a callbr with one successor will be considered a
proper loop preheader, which may result in instructions that use
the callbr return value being hoisted past it.
Fix this by adding callbr to isExceptionTerminator (with a rename
to isSpecialTerminator), which also fixes similar assumptions in
other places.
Fixes https://github.com/llvm/llvm-project/issues/64215.
Differential Revision: https://reviews.llvm.org/D158609
