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

[James Molloy]

Hi Bradley,

I have plenty of comments inline.

Cheers,

James

================
Comment at: lib/Target/AArch64/AArch64.h:43
@@ -42,2 +42,3 @@
 FunctionPass *createAArch64A57PBQPRegAlloc();
+FunctionPass *createAArch64FixCortexA53_835769();
 /// \brief Creates an ARM-specific Target Transformation Info pass.
----------------
I still maintain what I said internally that the name of the pass should fit convention - there's a convention in two lines above of putting "A5*" after AArch64. Can you please change to fit that convention unless you have a compelling reason otherwise?

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:29
@@ +28,3 @@
+#include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/CodeGen/RegisterScavenging.h"
+#include "llvm/CodeGen/RegisterClassInfo.h"
----------------
You don't need EquivalenceClasses, BitVector, or RegisterScavenging, (or raw_ostream).

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:52
@@ +51,3 @@
+static bool isFirstInstructionInSequence(MachineInstr *MI) {
+  // must return true if this instruction is a load, a store or a
+  // prefetch.
----------------
Capitalize "must".

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:69
@@ +68,3 @@
+static bool isSecondInstructionInSequence(MachineInstr *MI) {
+  // must return true for non-SIMD integer multiply-accumulates, writing
+  // to a 64-bit register.
----------------
All comments should have proper SPAG! :)

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:98
@@ +97,3 @@
+  const TargetRegisterInfo *TRI;
+  RegisterClassInfo RCI;
+
----------------
You're not using this, it should be removed.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:134
@@ +133,3 @@
+  MRI = &F.getRegInfo();
+  TRI = F.getRegInfo().getTargetRegisterInfo();
+  TII = TM.getSubtarget<AArch64Subtarget>().getInstrInfo();
----------------
You're not using this.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:163
@@ +162,3 @@
+
+static MachineInstr *getLastNonPseudo(MachineBasicBlock *MBB) {
+  for (auto I = MBB->rbegin(), E = MBB->rend(); I != E; ++I) {
----------------
Why does this return nullptr on failure instead of asserting?

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:177
@@ +176,3 @@
+  if (MI == &MBB.front()) {
+    MachineBasicBlock* PMBB = getBBFallenThrough(MBB);
+    assert(PMBB && "Expected basic block");
----------------
* is owned by the variable name: MachineBasicBlock *PMBB.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:200
@@ +199,3 @@
+
+  std::vector<MachineInstr*> SecondInstructionInFoundSequences;
+  unsigned Idx = 0;
----------------
GratuitouslyLongNameIsGratuitouslyLong. Wouldn't "Sequences" work, with a comment saying this is the terminating instruction in the sequence? Or something similarly shortened...

We like explicit names, but I feel this one is  a bit on the long side.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:205
@@ +204,3 @@
+  if (MachineBasicBlock *PMBB = getBBFallenThrough(MBB))
+      PrevInstr = getLastNonPseudo(PMBB);
+
----------------
What if PrevInstr is nullptr here? Will you have to jump back to the previous fallthrough if one exists? or assert?

It doesn't seem right to continue with PrevInstr being nullptr.

================
Comment at: lib/Target/AArch64/AArch64FixCortexA53_835769.cpp:232
@@ +231,3 @@
+  // Then update the basic block, inserting nops between the detected sequences.
+  for (auto &MI: SecondInstructionInFoundSequences) {
+    Changed = true;
----------------
Space after MI.

http://reviews.llvm.org/D5721