Page MenuHomePhabricator

[AArch64] Avoid pairing loads with same result reg
ClosedPublic

Authored by congzhe on Aug 31 2020, 9:34 PM.

Details

Summary

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.

Diff Detail

Event Timeline

congzhe created this revision.Aug 31 2020, 9:34 PM
congzhe requested review of this revision.Aug 31 2020, 9:34 PM
fhahn added a subscriber: fhahn.Sep 1 2020, 5:46 AM
fhahn added inline comments.
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
31

Is it possible to strip the IR references from the MIR test? Ideally most/all of the IR function here can be dropped.

188

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?

congzhe added inline comments.Sep 1 2020, 9:49 AM
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
188

Thanks for the comment!
The different functions do not test different scenarios. This is one single program (one single test) that exposes the bug, so probably all functions/BBs are still needed but I can try to reduce the program. I'm not very hopeful though because this test file is already a very much reduced version compared to its original version.

congzhe updated this revision to Diff 291086.Sep 10 2020, 2:39 PM
congzhe added a reviewer: fhahn.

Revision:

  • Fixed code style
  • Reduce the length of the test file
congzhe added inline comments.Sep 10 2020, 2:45 PM
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
31

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

188

Now reduced the size of the test file, resulting in only one function in the mir which exposes the LDR-to-LDP-conversion bug.

fhahn added inline comments.Sep 16 2020, 6:24 AM
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
31

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

...
congzhe updated this revision to Diff 292307.Sep 16 2020, 12:21 PM
congzhe added inline comments.Sep 16 2020, 12:24 PM
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
31

@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.

fhahn accepted this revision.Sep 17 2020, 1:55 AM

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.

This revision is now accepted and ready to land.Sep 17 2020, 1:55 AM
congzhe updated this revision to Diff 292459.Sep 17 2020, 4:25 AM
congzhe added inline comments.
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
13

@fhahn Thanks, now checking the full instruction sequence;

Also added another test with the order of LDRSW/LDRW flipped.

congzhe added inline comments.Sep 17 2020, 4:29 AM
llvm/test/CodeGen/AArch64/aarch64-ldst-subsuperReg-no-ldp.mir
13

Also made similar updates in the other patch
https://reviews.llvm.org/D86956

fhahn added inline comments.Sep 17 2020, 10:20 AM
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.

congzhe retitled this revision from [AArch64LdStOptimzation] fix a bug in AArch64 Load Store Optimization to [AArch64] Avoid pairing loads with same result reg.Sep 17 2020, 3:50 PM
congzhe edited the summary of this revision. (Show Details)

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.

Thanks, updated the commit message accordingly.

Great. Please let us know if you need someone to commit the change on your behalf.

congzhe updated this revision to Diff 293055.Sep 20 2020, 9:10 PM
This revision was automatically updated to reflect the committed changes.