[PATCH] D27329: AArch64CollectLOH: Rewrite as block-local analysis.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

Hi Matthias,

This looks indeed much simpler! That helps to narrow down the supported cases :).
LGTM, with a couple of nitpicks.
Also, the pass does not have any DEBUG statement now AFAICT. Could you add some?

Thanks,
-Quentin



================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:274
+/// State tracked per physical register.
+struct LOHInfo {
+  MCLOHType Type : 8;           ///< "Best" type of LOH possible.
----------------
Given how the code is structured, i.e., this definition and all the helper functions before the main algorithm, I would add a comment saying that this structure will be populated via a bottom up traversal of the basic blocks.
Then, a LOH will be recorded when we reach an ADRP with a candidate chain (or chains when we have the ADRP_ADRP case on top of the other).


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:278
+  bool OneUser : 1;             ///< Found exactly one user (yet).
+  bool MultiUsers : 1;          ///< Found multiple users.
+  const MachineInstr *MI0;      ///< First instruction involved in the LOH.
----------------
I wouldn't bother defining bitfield size.


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:328
+/// Update state \p Info given that \p MI is possibly the middle instruction
+/// of an LOH involving 3 instructions.
+static bool handleMiddleInst(const MachineInstr &MI, LOHInfo &DefInfo,
----------------
Typo -> s/an/a


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:355
+  } else {
+    assert(MI.getOpcode() == AArch64::LDRXui && "Expect LDRXui");
+    if (OpInfo.Type == MCLOH_AdrpAddStr && OpInfo.MI1 == nullptr) {
----------------
I'd also assert that the operand is a GOT entry.


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:505
+        break;
+      }
+      handleNormalInst(MI, LOHInfos);
----------------
Shouldn't we have a default case to avoid warnings?


================
Comment at: test/CodeGen/AArch64/arm64-collect-loh.ll:637
 ; CHECK: adrp [[ADRP_REG:x[0-9]+]], [[CONSTPOOL:lCPI[0-9]+_[0-9]+]]@PAGE
-; CHECK-NEXT: ldr q[[IDX:[0-9]+]], {{\[}}[[ADRP_REG]], [[CONSTPOOL]]@PAGEOFF]
+; CHECK: ldr q[[IDX:[0-9]+]], {{\[}}[[ADRP_REG]], [[CONSTPOOL]]@PAGEOFF]
 ; The tuple comes from the next instruction.
----------------
Why is this not on the next line anymore?


================
Comment at: test/CodeGen/AArch64/arm64-collect-loh.ll:670
+; CHECK-DAG: .loh AdrpLdrGot
+; CHECK-DAG: .loh AdrpLdrGot
   %mul.i.i.i = fmul double undef, 1.000000e-06
----------------
There shouldn't be any nondeterminism in the output, so I would rather stick to whatever order you get now.


Repository:
  rL LLVM

https://reviews.llvm.org/D27329