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%
MachineFunction's probably should not include a backreference to
the owning MachineModuleInfo. Most of these references were used
just to query the MCContext, which MachineFunction already directly
stores. Other contexts are using it to query the LLVMContext, which
can already be accessed through the IR function reference.
Fixes#82659
There are some functions, such as `findRegisterDefOperandIdx` and `findRegisterDefOperand`, that have too many default parameters. As a result, we have encountered some issues due to the lack of TRI parameters, as shown in issue #82411.
Following @RKSimon 's suggestion, this patch refactors 9 functions, including `{reads, kills, defines, modifies}Register`, `registerDefIsDead`, and `findRegister{UseOperandIdx, UseOperand, DefOperandIdx, DefOperand}`, adjusting the order of the TRI parameter and making it required. In addition, all the places that call these functions have also been updated correctly to ensure no additional impact.
After this, the caller of these functions should explicitly know whether to pass the `TargetRegisterInfo` or just a `nullptr`.
Remove getSizeOrUnknown call when MachineMemOperand is created. For Scalable
TypeSize, the MemoryType created becomes a scalable_vector.
2 MMOs that have scalable memory access can then use the updated BasicAA that
understands scalable LocationSize.
Original Patch by Harvin Iriawan
Co-authored-by: David Green <david.green@arm.com>
This is part of #70452 that changes the type used for the external
interface of MMO to LocationSize as opposed to uint64_t. This means the
constructors take LocationSize, and convert ~UINT64_C(0) to
LocationSize::beforeOrAfter(). The getSize methods return a
LocationSize.
This allows us to be more precise with unknown sizes, not accidentally
treating them as unsigned values, and in the future should allow us to
add proper scalable vector support but none of that is included in this
patch. It should mostly be an NFC.
Global ISel is still expected to use the underlying LLT as it needs, and
are not expected to see unknown sizes for generic operations. Most of
the changes are hopefully fairly mechanical, adding a lot of getValue()
calls and protecting them with hasValue() where needed.
This is a small part of #70452, attempting to take a small simpler part
of it in isolation to simplify what remains. It changes the getSpillSize,
getFoldedSpillSize, getRestoreSize and getFoldedRestoreSize methods to return
optional<LocationSize> instead of unsigned. The code is intended to be the
same, keeping the optional<> to specify when there was no size found, with some
minor adjustments to make sure that unknown (~UINT64_C(0)) sizes are handled
sensibly. Hopefully as more unsigned's are converted to LocationSize's the use
of ~UINT64_C(0) can be cleaned up too.
We were allowing extra immediate arguments, and only bothering to check
if registers were implicit or not.
Also consolidate extra operand checks in verifier, to make this
testable. We had 3 different places checking if you were trying to build
an instruction with more operands than allowed by the definition. We had
an assertion in addOperand, a direct check in the MIRParser to avoid the
assertion, and the machine verifier checks. Remove the assert and parser
check so the verifier can provide a consistent verification experience,
which will also handle instructions modified in place.
