`getTypeToPrint` omits repeated generic types even on register
definitions. For instructions with multiple definitions sharing a type
index, such as `G_SDIVREM` and `G_FSINCOS`, this produces MIR that the
parser rejects with "generic virtual registers must have a type".
Assisted-by: GPT-6
A virtual use operand, tied to a def of a different register, sharing
the register with another read, carries obligation that is hard to
follow by passes. For example, twoaddressinstruction would use different
physical registers, breaking the expectation of the `IsUndef` flag:
```
define void @f() {
%y = freeze i32 undef
%m = mul i32 %y, %y
call void asm sideeffect "# use $0", "{cx}"(i32 %m)
ret void
}
%2 = IMUL32rr undef %1(tied-def 0), undef %1 ; before twoaddressinstruction
%2 = IMUL32rr undef %2(tied-def 0), undef %1 ; after
imull %eax, %ecx ; different regs
```
RegAllocFast had the same issue until #222249. Let's just reject the
shape to retire the obligation. `%0 = XOR undef %0, undef %0` stays
legal, since rewriting that tie changes nothing.
ProcessImplicitDefs and DetectDeadLanes stop producing the rejected
form: such reads keep their IMPLICIT_DEF. Physical registers are
unaffected, so post-RA expansions like Expand2AddrUndef keep
`$eax = XOR32rr undef $eax, undef $eax`.
Aided by Opus 5
Replace the use of getPointerRegClass to determine the register
class from an asm memory constraint, working to delete it. When
there are multiple pointer types there can't be just a single
answer.
Co-authored-by: Claude (Opus 4.8) <noreply@anthropic.com>
This optimizes MachineInstr::insert by copying trailing operands
first and then removing them from the end. This avoids repeatedly
shifting the remaining operands, reducing the operation from quadratic
to linear time without changing behavior.
Plumb the nonnull flag through to the backend so a flagged addrspacecast
lowers without the runtime null check, matching what
llvm.amdgcn.addrspacecast.nonnull already provides.
Add the NonNull MIFlag with MIR printer/parser support (including the
MIRPrinter path and update_mir_test_checks) so it round-trips on
G_ADDRSPACE_CAST, and preserve it through SelectionDAG vector
scalarization and splitting.
Co-authored-by: Claude (Claude-Opus-4.8) <noreply@anthropic.com>
The loop only inspects the using instruction, so iterate instructions
directly instead of operands and their parents.
Co-authored-by: Claude (Claude-Opus-4.8)
PR #210028 added a run of EarlyMachineLICM before StackColoring removed
LIFETIME_START/LIFETIME_END markers, and it ended up hoisting them which
led to miscompiles. This marks them as not safe to move in
`MachineInstr::isSafeToMove`.
---------
Co-authored-by: Matt Arsenault <arsenm2@gmail.com>
The LRSplit MachineInstr flag is set by SplitKit on copies inserted for
live-range splitting.
Until now the flag had no MIR-text representation.
This patch fixes that so that it gets easier to reproduce/capture issues
that involves SplitKit.
Round-trip coverage in
llvm/test/CodeGen/MIR/AMDGPU/lr-split-flag.mir.
TrackingMDNodeRef is expensive and the tracking functionality is only
used when parsing textual LLVM IR. Therefore, store a plain DILocation
pointer in DebugLoc and update the debug locs explicitly when parsing
finishes.
Invalid debug metadata now fails directly at parsing and not (just)
later when verifying. A consequence is that old-style DILocations cannot
be parsed from textual IR anymore.
As related cleanup, remove the now-unused hasTrivialDestructor() on
TrackingMDRef.
While work on changing DILocation to no longer be metadata is on the
way, it is going to take a while to finish, we can get this immediate
performance and max-rss improvement earlier.
Unlike LLVM IR `Instruction::eraseFromParent()`,
`MachineInstr::eraseFromParent()` is void and does not return the
iterator following the erased instruction. Returning an iterator can be
very helpful for example when we are erasing MachineInstrs while
iterating, as it provides a convenient way to get a valid iterator.
This patch updates `MachineInstr::eraseFromParent()` to return a
`MachineBlock::iterator` (which is a
`MachineInstrBundleIterator<MachineInstr>`). If the erased instruction
is the head of a bundle, then the returned iterator points to the next
bundle (see unittest).
- Rename `getTiedOperandIdx` to `GetTiedOperandIdx` per LLVM CS.
- Do not compute tied operand index for defs, since tied operands are
printed only on uses.
- Restructure the `if` in the later operand printing loop to not compute
tied operand index/type for subreg-index imm operands.
As discussed on https://github.com/llvm/llvm-project/pull/144745, insert
a nop after unwinding inline assembly, as it may end on a call.
While the change itself is trivial, I ended up having to do two
infrastructure changes:
* The unwind flag needs to be propagated to ExtraInfo of the
MachineInstr.
* The MachineInstr needs to be passed through to emitInlineAsmEnd(), and
the method needs to be non-const.
Fixes https://github.com/llvm/llvm-project/issues/157073.
We have hit the limit of 24 bits for LLVM_MI_FLAGS_BITS in upstream. In
our downstream target we are using additional bits, so we're hitting the
"Flag is out of range" assertions in get/setFlag functions.
Increase LLVM_MI_FLAGS_BITS to 32 bits, and reorder the fields of
MachineInstr to avoid extra padding and the subsequent increase in size.
Since the alignment of the class is 8 and the last member was 16-bit
before this change we can get more bits by moving things around.
When trying to enable AArch64 Import Call Optimization for Windows, we
noticed an issue where a call to an incorrect function was happening
after the loader replaced a branch instruction. The root cause of this
was that LLVM had decided to fold two branch instructions into one as
they were both branches to the same register, however the value of the
register would be different in either path as they were branches for
different imported functions.
This change updates `MachineInstr::isIdenticalTo` to also consider any
"called global" that is attached to the instruction, and will consider
two instructions as "not the same" if the globals differ.
Also fixed a possible source of non-determinism: switched from using a
`DenseMap` to using a `vector` for mapping sections to lists of called
globals (we don't expect many sections, so no need to use a map) and
sort the map by section name before emitting.
As per @arsenm 's instructions, I've separated the non-functional
changes from https://github.com/llvm/llvm-project/pull/169958.
Afterwards I'll tackle the functional ones one by one. I hope I did
everything right this time.
Full descriptions in the article:
https://pvs-studio.com/en/blog/posts/cpp/1318/
3. Array overrun is possible.
The PVS-Studio warning: V557 Array overrun is possible. The value of
'regIdx' index could reach 31. VEAsmParser.cpp 696
10. Excessive check.
The PVS-Studio warning: V547 Expression 'IsLeaf' is always false.
PPCInstrInfo.cpp 419
11. Doubling the same check.
The PVS-Studio warning: V581 The conditional expressions of the 'if'
statements situated alongside each other are identical. Check lines:
5820, 5823. PPCInstrInfo.cpp 5823
15. Excessive check.
The PVS-Studio warning: V547 Expression 'i != e' is always true.
MachineFunction.cpp 1444
17. Excessive assignment.
The PVS-Studio warning: V1048 The 'FirstOp' variable was assigned the
same value. MachineInstr.cpp 1995
18. Excessive check.
The PVS-Studio warning: V547 Expression 'AllSame' is always true.
SimplifyCFG.cpp 1914
19. Excessive check.
The PVS-Studio warning: V547 Expression 'AbbrevDecl' is always true.
LVDWARFReader.cpp 398
There can only be meaningful aliasing between the memory accesses of
different instructions if at least one of the accesses modifies memory.
This check is applied at the instruction-level earlier in the method.
This change merely extends the check on a per-MMO basis.
This affects a SystemZ test because PFD instructions are both mayLoad
and mayStore but may carry a load-only MMO which is now no longer
treated as aliasing loads. The PFD instructions are from llvm.prefetch
generated by loop-data-prefetch.
getPointerRegClass is a layering violation. Its primary purpose
is to determine how to interpret an MCInstrDesc's operands RegClass
fields. This should be context free, and only depend on the subtarget.
The model of this is also wrong, since this should be an
instruction / operand specific property, not a global pointer class.
Remove the the function argument to help stage removal of this hook
and avoid introducing any new obstacles to replacing it.
The remaining uses of the function were to get the subtarget, which
TargetRegisterInfo already belongs to. A few targets needed new
subtarget derived properties copied there.
This flag applies to G_PTR_ADD instructions and indicates that the operation
implements an inbounds getelementptr operation, i.e., the pointer operand is in
bounds wrt. the allocated object it is based on, and the arithmetic does not
change that.
It is set when the IRTranslator lowers inbounds GEPs (currently only in some
cases, to be extended with a future PR), and in the
(build|materialize)ObjectPtrOffset functions.
Inbounds information is useful in ISel when we have instructions that perform
address computations whose intermediate steps must be in the same memory region
as the final result. A follow-up patch will start using it for AMDGPU's flat
memory instructions, where the immediate offset must not affect the memory
aperture of the address.
This is analogous to a concurrent effort in SDAG: #131862
(related: #140017, #141725).
For SWDEV-516125.
- Change MachineInstr operand accessors to use `ArrayRef` internally to
slice the operand array into sub-arrays.
- Minor: remove unnecessary {} on `MachineInstrBuilder::add`.
I noticed these destructors taking time with -ftime-trace and moved some
of them for minor build efficiency improvements.
The main impact of moving destructors out of line is that it avoids
requiring container fields containing other types from being complete,
i.e. one can have uptr<T> or vector<T> as a field with an incomplete
type T, and that means we can reduce transitive includes, as with
LegalizerInfo.h.
Move expensive getDebugOperandsForReg template out-of-line. The
std::function instantiation shows up in time trace even if you don't use
the function.
The primary effect of this is that we get proper scalable sizes printed
by the assembler, but this may also enable proper aliasing analysis. I
don't see any test changes resulting from the later.
Getting the size is slightly tricky as we store the scalable size as a
non-scalable quantity in the object size field for the frame index. We
really should remove that hack at some point...
For the synthetic tuple spills and fills, I dropped the size from the
split loads and stores to avoid incorrect (overly large) sizes. We could
also divide by the NF factor if we felt like writing the code to do so.
Even if an inline asm doesn't have memory effects, we can't assume it's
safe to speculate: it could trap, or cause undefined behavior. At the
LLVM IR level, this is handled correctly: we don't speculate inline asm
(unless it's marked "speculatable", but I don't think anyone does that).
Codegen also needs to respect this restriction.
This change stops Early If Conversion and similar passes from
speculating an INLINEASM MachineInstr.
Some uses of isSafeToMove probably could be switched to a different API:
isSafeToMove assumes you're hoisting, but we could handle some forms of
sinking more aggressively. But I'll leave that for a followup, if it
turns out to be relevant.
See also discussion on gcc bugtracker
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102150 .
[[Change-Id:
Ic349022bb99ef91f5396e462ade0366bc772ae02](https://github.com/llvm/llvm-project/pull/123531)](https://github.com/llvm/llvm-project/pull/123531)
moved isDead() from DeadMachineInstrElim to MachineInstr . In the
process of moving, I reordered the checks to improve chances of early
exit, but this has caused a slight increase in compile time.
This PR reverts back to the original order of checks.
Provide isDead interface for access to ad-hoc isDead queries.
LivePhysRegs is optional: if not provided, pessimistically check
deadness of a single MI without doing the LivePhysReg walk; if provided
it is assumed to be at the position of MI.
Once we get to SelectionDAG the IR should not be changing anymore, so we
can use BatchAAResults rather than AAResults to cache AA queries.
This should be a NFC change for targets that enable AA during codegen
(such as AArch64), but also give a nice compile-time improvement in some
cases. See:
https://github.com/llvm/llvm-project/pull/123787#issuecomment-2606797041
Note: This follows Nikita's suggestion on #123787.
Fixes the "use after poison" issue introduced by #121516 (see
<https://github.com/llvm/llvm-project/pull/121516#issuecomment-2585912395>).
The root cause of this issue is that #121516 introduced "Called Global"
information for call instructions modeling how "Call Site" info is
stored in the machine function, HOWEVER it didn't copy the
copy/move/erase operations for call site information.
The fix is to rename and update the existing copy/move/erase functions
so they also take care of Called Global info.
Currently LLVMContext::emitError emits any error as an "inline asm"
error which does not make any sense. InlineAsm appears to be special,
in that it uses a "LocCookie" from srcloc metadata, which looks like
a parallel mechanism to ordinary source line locations. This meant
that other types of failures had degraded source information reported
when available.
Introduce some new generic error types, and only use inline asm
in the appropriate contexts. The DiagnosticInfo types are still
a bit of a mess, and I'm not sure why DiagnosticInfoWithLocationBase
exists instead of just having an optional DiagnosticLocation in the
base class.
DK_Generic is for any error that derives from an IR level instruction,
and thus can pull debug locations directly from it. DK_GenericWithLoc
is functionally the generic codegen error, since it does not depend
on the IR and instead can construct a DiagnosticLocation from the
MI debug location.
Merge GlobalISel's isTriviallyDead and DeadMachineInstructionElim's
isDead code and remove all unnecessary checks from the hot path by
looping over the operands before doing any other checks.
See #105950 for why DeadMIElim needs to remove LIFETIME markers even
though they probably shouldn't generally be considered dead.
x86 CTMark O3: -0.1%
AArch64 GlobalISel CTMark O0: -0.6%, O2: -0.2%
Alex MacLean