Page MenuHomePhabricator

[AArch64] Avoid pairing loads when the base reg is modified
ClosedPublic

Authored by congzhe on Sep 1 2020, 9:39 AM.

Details

Summary

When pairing loads, we should check if in between the two loads the
base register has been modified. If that is the case then avoid pairing
them because the second load actually loads from a different address.

Diff Detail

Event Timeline

congzhe created this revision.Sep 1 2020, 9:39 AM
congzhe requested review of this revision.Sep 1 2020, 9:39 AM
fhahn requested changes to this revision.Sep 1 2020, 9:42 AM

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

This revision now requires changes to proceed.Sep 1 2020, 9:42 AM

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

Thanks! Regarding the MIR test: similar to the test file in the other patch https://reviews.llvm.org/D86906, the MIR test file for this patch is very lengthy (over 1000 lines). I'm wondering if it looks fine if I use that test file, or I could spend time trying to prune it, although I'm not optimistic how much it can be reduced.

congzhe updated this revision to Diff 291090.Sep 10 2020, 2:47 PM

Revision:

  • Fixed code style
  • Provided an mir test

Is it possible to add a MIR test for the issue? Also, it looks like the formatting is off a bit. Could you run clang-format-diff on the change?

Thanks for the comment, now fixed the code style and provided a test case which has already been reduced using creduce and llvm-reduce.

congzhe updated this revision to Diff 292308.Sep 16 2020, 12:21 PM
congzhe updated this revision to Diff 292360.Sep 16 2020, 3:10 PM
congzhe updated this revision to Diff 292460.Sep 17 2020, 4:28 AM
fhahn added inline comments.Sep 17 2020, 10:44 AM
llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir
18

It might be good to have test cases with stores & non-load instructions modifying the base?

congzhe retitled this revision from [AArch64LoadStoreOptimization] Bug fix in ldr to ldp conversion to [AArch64] Avoid pairing loads when the base reg is modified.Sep 17 2020, 3:58 PM
congzhe edited the summary of this revision. (Show Details)
congzhe updated this revision to Diff 293056.Sep 20 2020, 9:11 PM
congzhe added inline comments.Sep 20 2020, 9:28 PM
llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir
18

Thanks for this comment, now added test cases where the base register or base address is modified with non-load instructions.
However, the bug in AArch64 Load Store Optimization pass only exists when the base register is updated with load instructions in between two loads for pairing, i.e.,

ldr x9 [x10]
ldr x10 [x8]
ldr x10 [x10, 8].

For other instructions that modify the base register or base address with non-load instructions, the existing pass works correctly. The case that the base register is modified with non-load instructions is handled in lines 1626-1627 of the original AArch64 Load Store Optimization pass:

if (!ModifiedRegUnits.available(BaseReg))
  return E;

Only when the pattern shown above occurs, this pass fails to handle it because it would hit a continue in the for-loop and would not reach lines 1626-1627.

Still, I added these test cases where the base register or base address is modified with non-load instructions, but please note that these test cases already work correctly without the patch since the original AArch64 Load Store Optimization pass handles them well. I did not find similar tests in other test files that's why I added them, but maybe not adding them also makes sense. I'll appreciate it if you could let me know your thoughts.

congzhe added a comment.EditedSep 29 2020, 9:44 PM

@fhahn pinging reviewers :)
Comments are very much appreciated.

fhahn requested changes to this revision.Sep 30 2020, 3:54 AM
fhahn added inline comments.
llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1585

then *we* cannot do the optimization?

1591

can we hoist the check outside of the containing if, i.e. to around line 1562? I think we should be able to bail out once the base reg is modified, because it won't get 'un-modified' so that should not rule out any valid pairs.

Also, it is safer to do it earlier, otherwise we would need the check for each code path that returns a valid found pair (for example, we would probably also need it around line 1611)

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir
4

nit: spurious to, should that be tries to convert load instructions?

5

a ldp instruction?

6

convertion -> conversion?

18

Thanks for adding those tests! Even though it is already handled correctly, adding a few additional tests here for completeness makes sense to me.

This revision now requires changes to proceed.Sep 30 2020, 3:54 AM
congzhe updated this revision to Diff 295273.Sep 30 2020, 7:20 AM
fhahn accepted this revision.Sep 30 2020, 7:22 AM

LGTM, thanks!

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1568

nit: capitalize For.

This revision is now accepted and ready to land.Sep 30 2020, 7:22 AM

Thanks for all the comments, now addressed all of them @fhahn

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1585

Thanks, updated accordingly.

1591

Now hoisted the check outside of the containing if.

llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir
6

Thanks for the comments, revised accordingly.

congzhe updated this revision to Diff 295307.Sep 30 2020, 8:50 AM
congzhe updated this revision to Diff 295310.Sep 30 2020, 8:55 AM
This revision was automatically updated to reflect the committed changes.