[PATCH] [AArch64] Add workaround for Cortex-A53 erratum (835769)

[Tim Northover]

Hi Bradley,

Thanks for being open about the issue, it's really excellent to see ARM contributing to LLVM even in difficult circumstances like this.

I can't really comment on the actual work around, since I haven't seen the erratum; but I'll trust you've had enough look at it. I did spot a couple of issues; one stylistic, the other a bit more problematic.

Cheers.

Tim.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:34-37
@@ +33,6 @@
+
+static cl::opt<bool>
+WorkAroundA53Erratum("aarch64-fix-cortex-a53-835769", cl::Hidden,
+                     cl::desc("Work around Cortex-A53 erratum 835769"),
+                     cl::init(false));
+
----------------
I think we're trying to standardise on putting these predicates in AArch64TargetMachine.h, to decide whether the pass is even added.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:143
@@ +142,3 @@
+  for (MachineBasicBlock *S : MBB.predecessors())
+    if (S == PrevBB)
+      return S;
----------------
This doesn't necessarily mean it was a fallthrough. Particularly at -O0 a block may happen to be before another in layout order but contain a real branch anyway.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:168
@@ +167,3 @@
+    DebugLoc DL = I->getDebugLoc();
+    BuildMI(PMBB, DL, TII->get(AArch64::HINT)).addImm(0);
+  }
----------------
I think this may damage expected block semantics too. You're putting an instruction without isTerminator after the real terminators (like the conditional branch).

I'd hope the verifier would complain about that (but be unsurprised if it didn't). Other passes may not cope (in particular, a glance at AnalyzeBranch suggests it'll fail after this).

Are you using the previous block to win back some efficiency when there's multiple branches here? If so, it may not be worth the effort, though I'll leave you to decide.

http://reviews.llvm.org/D5721