[PATCH] D103597: [AArch64LoadStoreOptimizer] Generate more STPs by renaming registers earlier

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1707
+        // being generated yet.
+        if (TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg())) {
+          bool flagsHaveRenameReg = updateFlagsWithRenameReg(
----------------
Is there a reason to do the whole check for rename registers twice, or could we determine whether re-naming is needed once for all cases? In particular, by doing it at this state, we may miss out on some legality checks, e.g. the one at line 1734 to check if the base register has been modified? 

Can you add a test case where the base is modified?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1708
+        if (TRI->isSuperOrSubRegisterEq(Reg, getLdStRegOp(MI).getReg())) {
+          bool flagsHaveRenameReg = updateFlagsWithRenameReg(
+              MaybeCanRename, Flags, FirstMI, MI, DefinedInBB, UsedInBetween,
----------------
nit: I don't think this matches LLVM's coding style (should be camel case with upper case start, https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:548
+
+# During ISel, the order of load/store pairs can be optimized and changed
+# which prevents STPs from being generated. These tests check that an STP 
----------------
IIUC the *real* difference between the existing tests in the file and the new ones is that in the new ones, the merge-able `STR` instructions have the same register as stored operand? If that's the case, it would be good to clarify the comment, as in the other cases things could also originate from re-ordering.


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:554
+#
+# CHECK-LABEL: name: memcpy32
+# CHECK: liveins: $x0, $x1
----------------
can you also add a negative test, like one where there is no register available for renaming? I think you can do that by adding a couple of live-ins.


================
Comment at: llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir:564
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
----------------
please reduce the values here, most are not needed (see the tests above).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103597