When pairing ldr instructions to an ldp instruction, we cannot pair two ldr
destination registers where one is a sub or super register of the other.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 30 | Is it possible to strip the IR references from the MIR test? Ideally most/all of the IR function here can be dropped. | |
| 187 | Are all the loads/stores and the different BBs needed for the test? Also, are the different functions needed or do they test different scenarios? | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 187 | Thanks for the comment! | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 30 | I spent effort trying to drop out the IR, but since some global variables such as @_MergedGlobals or @i are used in the mir, if I were to drop the IR llc would report undefined global variables error. Is there a way that I can drop the IR while still being able to run the mir? @fhahn | |
| 187 | Now reduced the size of the test file, resulting in only one function in the mir which exposes the LDR-to-LDP-conversion bug. | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 30 | I think it should be possible to get rid of the IR references by re-writing the address computations to use the same base register but with different offsets to ensure it is clear there is no aliasing going on. Reduced test case: ---
name: test
tracksRegLiveness: true
body: |
bb.0:
liveins: $x9, $x11
renamable $w10 = LDRWui renamable $x9, 2 :: (load 4)
STRWui killed renamable $w10, renamable $x9, 2 :: (store 4)
renamable $x10 = LDRSWui renamable $x9, 3 :: (load 4)
STRWui renamable $w11, renamable $x9, 8 :: (store 4)
renamable $w10 = LDRWui renamable $x9, 2 :: (load 4)
RET undef $lr, implicit undef $w0
... | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 30 | @fhahn Thank you very much for this comment, now further reduced this mir test case. Also the mir test case in https://reviews.llvm.org/D86956 has been further reduced. | |
LGTM, thanks!
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 13 | Given that the test is very short now, could you check for the full instruction sequence (LDR, LDR, RET)? Just to make sure the loads are not dropped completely (as $w10 is not really used any more)/. Also, it might eb good to add another test with the order of LDRSW/LDRW flipped. | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 13 | Also made similar updates in the other patch | |
| llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir | ||
|---|---|---|
| 13 | Thanks, that looks good! | |
oh, it might be good to slightly adjust the commit message to be a bit more compact and also mention what actual bug is fixed, e.g. something like [AArch64] Avoid paring loads with same result reg.
clang-format: please reformat the code
- if (MayLoad && TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg())) { + if (MayLoad && + TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg())) {