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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB marked 7 inline comments as done.
MatzeB added a comment.

Thanks for the review. I added some debug statements and spend some time creating a .mir test for all situations happening in the code (and fixed a bug discovered while doing so).



================
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,
----------------
qcolombet wrote:
> Typo -> s/an/a
I imagine this as "an 'el' 'oh' 'haitch'"


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


================
Comment at: lib/Target/AArch64/AArch64CollectLOH.cpp:505
+        break;
+      }
+      handleNormalInst(MI, LOHInfos);
----------------
qcolombet wrote:
> Shouldn't we have a default case to avoid warnings?
We're switching over `unsigned Opcode` and don't get a warning.


================
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.
----------------
qcolombet wrote:
> Why is this not on the next line anymore?
This example contained a perfectly fine LOH opportunity that wasn't catched before (probably because the old algorithm bailed out on the double q reg and didn't resume properly on the LDR address). I updated the test to make it apparent.


================
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
----------------
qcolombet wrote:
> There shouldn't be any nondeterminism in the output, so I would rather stick to whatever order you get now.
ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D27329