[PATCH] [AArch64] Override useAA and remove unneeded edges in the scheduling graph. (Phabricator)

[James Molloy]

Hi Sanjin,

I have a bunch of comments, but this patch looks in good shape already! One other thing, it looks like you're adding quite a few blank lines between functions. There should be one blank line only. A nitpick I know, but nitpicks like this keep the codebase in decent shape.

Cheers,

James

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1195
@@ -1194,1 +1194,3 @@
 
+  // areMemAccessesTriviallyDiff - Sometimes, it is possible for the target to
+  // tell, even without aliasing information, that two MIs access different
----------------
I don't like the truncation of "different" to "diff". In fact, I think "disjoint" would be more accurate: "areMemAccessesTriviallyDisjoint".

What is the behaviour is MIa or MIb are not memory access instructions? you've below had them return false but I think it should assert in this case.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1204
@@ +1203,3 @@
+
+  /// For instructions with a base and offset, return the position of the
+  /// base register and offset operands.
----------------
Unused function.

================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:521
@@ -517,1 +520,3 @@
 
+  if (TII->areMemAccessesTriviallyDiff(MIa, MIb))
+    return false;
----------------
Either you need to check if these are memory accesses first, or you should rename the function or make it explicit that it will be well behaved on non-memory operands. With its current name, one would expect that a precondition is the arguments are memory instructions.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:617
@@ +616,3 @@
+
+  if (!MIa->mayStore() && !MIa->mayLoad())
+    return false;
----------------
As mentioned above, unintuitive.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:631
@@ +630,3 @@
+      int LowOffset = OffsetA < OffsetB ? OffsetA : OffsetB;
+      int HighOffset = (LowOffset == OffsetA) ? OffsetB : OffsetA;
+      int LowWidth = (LowOffset == OffsetA) ? WidthA : WidthB;
----------------
For clarity, I'd prefer if you used the same condition in each ternary instead of chaining off the first condition:

    int LowOffset = OffsetA < OffsetB ? ...
    int HighOffset = OffsetA < OffsetB ? ...

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:1391
@@ +1390,3 @@
+
+  if (!LdSt->getOperand(1).isReg() || !LdSt->getOperand(2).isImm())
+     return false;
----------------
Probably best to early-exit this before the switch.

================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:114
@@ -113,1 +113,3 @@
 
+  bool useAA() const override { return true; }
+
----------------
This should go in in a separate commit before the rest of the code as it is orthogonal.

================
Comment at: test/CodeGen/AArch64/arm64-triv-diff-mem-access.ll:2
@@ +1,3 @@
+; REQUIRES: asserts
+; RUN: llc < %s -mtriple=arm64-linux-gnu -mcpu=cortex-a53 -enable-misched -verify-misched -debug-only=misched -o - 2>&1 > /dev/null | FileCheck %s
+; Check that there is no edge created between stores that use the same base
----------------
Do we need -enable-misched here? isn't it enabled by default?

================
Comment at: test/CodeGen/AArch64/arm64-triv-diff-mem-access.ll:6
@@ +5,3 @@
+
+; CHECK: Reject chain dep
+; Function Attrs: nounwind
----------------
Tests that check debug output are generally frowned upon, unless there is no better way to write them.

I assume there is no better way to write this?

http://reviews.llvm.org/D5103