[PATCH] D86956: [AArch64] Avoid pairing loads when the base reg is modified

[Florian Hahn via Phabricator via llvm-commits]

fhahn requested changes to this revision.
fhahn added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1572
 
+          // If the BaseReg has been modified, then cannot do the optimization.
+          // for example, in the following pattern
----------------
then *we* cannot do the optimization?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1578
+          // the first and third ldr cannot be converted to ldp x1, x4, [x2]
+          if (!ModifiedRegUnits.available(BaseReg))
+            return E;
----------------
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)


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir:3
+#
+# When the AArch64 Load Store Optimization pass tries to convert to load instructions
+# into an ldp instruction, and when the base register of the second ldr instruction
----------------
nit: spurious `to`, should that be `tries to convert load instructions`?


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir:4
+# When the AArch64 Load Store Optimization pass tries to convert to load instructions
+# into an ldp instruction, and when the base register of the second ldr instruction
+# has been modified in between these two ldr instructions, the convertion should not
----------------
`a ldp` instruction?


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir:5
+# into an ldp instruction, and when the base register of the second ldr instruction
+# has been modified in between these two ldr instructions, the convertion should not
+# occur.
----------------
`convertion` -> `conversion`?


================
Comment at: llvm/test/CodeGen/AArch64/aarch64-ldst-modified-baseReg.mir:17
+# CHECK: $x10 = LDURXi $x8, 1 :: (load 8)
+# CHECK: $x10 = LDRXui $x10, 0 :: (load 8)
+# CHECK: RET
----------------
congzhe wrote:
> fhahn wrote:
> > It might be good to have test cases with stores & non-load instructions modifying the base?
> 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.
> 
Thanks for adding those tests! Even though it is already handled correctly, adding a few additional tests here for completeness makes sense to me.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86956/new/

https://reviews.llvm.org/D86956