[PATCH] D35366: [AArch64][Falkor] Avoid HW prefetcher tag collisions (step 2)

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:188
+
+// Bits from load opcodes used to compute HW prefetcher instruction tags.
+struct LoadInfo {
----------------
`///`


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:214
+
+static Optional<LoadInfo> getLoadInfo(const MachineInstr &MI) {
+  int DestRegIdx;
----------------
I generally prefer long instruction list being in AArch64InstrInfo. I think a good guide is asking yourself, what happens if someone adds a new Load/Store instruction: does this code need to be adapted and will the person adding the load/store be able to find this code?

AArch64InstrInfo, also already has various getMemXXX functions, at a first glance it looks like code duplication. Did you check whether this code could use the existing functions or the existing functions could be rewritten using this code?


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:684
+  // loads to avoid collisions with any other loads.
+  RegScavenger RS;
+  for (auto *MBB : L.getBlocks()) {
----------------
It looks like you only need the scavenger to find free registers. In this case I'd recommend using the slightly leaner/simpler `LiveRegUnits` class instead:
- `addLiveOuts()` correctly handles pristines registers already.
- You can iterate over the register class directly testing 
`available(Reg) && !MRI.isReserved(Reg)` which avoids constructing intermediate bitsets (and hence saves you some malloc/free operations).


================
Comment at: lib/Target/AArch64/AArch64FalkorHWPFFix.cpp:685-687
+  for (auto *MBB : L.getBlocks()) {
+    RS.enterBasicBlockEnd(*MBB);
+    for (auto I = MBB->rbegin(); I != MBB->rend(); RS.backward(), ++I) {
----------------
Less `auto` please.


================
Comment at: test/CodeGen/AArch64/falkor-hwpf-fix.mir:30
+...
+# Verify that the tag collision between the loads is resolved and written back for post increment addressing.
+# CHECK-LABEL: name: hwpf2
----------------
I think this should be surrounded by `---`, `...`; looks like the llvm yaml parser is lenient.


https://reviews.llvm.org/D35366