[PATCH] D94949: [AArch64][RegAllocFast] Add findSpillBefore to TargetRegisterInfo

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

Some comments below, in addition to these questions

- Have you tested this with both the new and the legacy pass managers?

I think the most important question to be answered about the approach is whether the backends use bundles anywhere else - if it does, this is probably too brittle and pseudo-instructions is a better approach, even though it adds duplication of loop insertion.



================
Comment at: llvm/include/llvm/CodeGen/AtomicLoopBundler.h:42
+    MachineBasicBlock::instr_iterator LdIter =
+        std::find_if(MBB.instr_begin(), MBB.instr_end(), Derived::IsLoadInstr);
+
----------------
Can you document the use of Derived::IsLoadInstr and Derived::IsStoreInstr? It's not clear from a quick scan of the class that they are required to use the pass.


================
Comment at: llvm/include/llvm/CodeGen/AtomicLoopBundler.h:57-58
+    // Search for the exclusive store
+    MachineBasicBlock::instr_iterator StrIter =
+        std::find_if(MBB.instr_begin(), MBB.instr_end(), Derived::IsStoreInstr);
+
----------------
Don't you want to find the store *after* the Load? So maybe start at `LdIter`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:613
+  // inside the bundle will not be lowered correctly.
+  addPass(createUnpackMachineBundles(nullptr));
+}
----------------
Does the backend use bundles anywhere else, as we would need to make sure we're not unpacking other bundles at this point by mistake.


================
Comment at: llvm/lib/Target/ARM/ARMAtomicLoopBundler.cpp:18
+
+// TODO add LoadAquire/StoreRelease instructions?
+bool ARMAtomicLoopBundler::IsLoadInstr(MachineInstr &MI) {
----------------
Here you're looking for instructions as introduced by atomic loop expansion - so the predicate should match that (I think).

Alternatively, you could just use `MI.mayLoad()` if you're looking for any kind of Load?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94949/new/

https://reviews.llvm.org/D94949