[PATCH] D109052: [AMDGPU][GlobalISel] Fix waterfall loops

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Localizer.cpp:172
 
-    MachineBasicBlock::iterator II(MI);
+    MachineBasicBlock::instr_iterator II(MI);
     ++II;
----------------
foad wrote:
> Why is this change needed? Normally a BUNDLE instruction has use operands representing all the uses of the instructions inside the bundle, so I think this code should just work without any changes? See comment in lowerCall...
As far as I understand, it only has that after register allocation (or somewhere in these passes, like phi elimination. The pass that adds the BUNDLE instruction also adds the uses and defs).


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2273
+    if (!regsLive.count(Reg) &&
+        !RegSet(regsDefined.begin(), regsDefined.end()).count(Reg)) {
       if (Register::isPhysicalRegister(Reg)) {
----------------
foad wrote:
> Why is this change needed?
> 
> Anyway I think you can write `!is_contained(regsDefined, Reg)`.
The MachineVerifier sees the whole bundle as a single instruction. regsLive will only be updated after the whole bundle is worked on. This makes sense for VLIW architectures like Hexagon but doesn’t make sense for grouping instructions in amdgpu.
So, making this target specific is probably the ideal way, maybe I should do this? This currently relaxes the check to allow uses to refer to defines that happen in the same bundle.

Thanks, `is_contained` looks useful.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp:1426
+    auto &MBB = MIRBuilder.getMBB();
+    MIBundleBuilder Bundler(MBB, CallStart.getValue(),
+                            MIRBuilder.getInsertPt());
----------------
foad wrote:
> Use finalizeBundle instead of MIBundleBuilder, to add all the required operands to the BUNDLE instruction?
As far as I understood, finalizeBundle should only be used after register allocation. This will be done in some pass there, insert the BUNDLE instruction and add the uses and defs. Adding the uses and defs here will fail the MachineVerifier, because you can only have a single def in SSA mode.

The bundle will be dissolved before it gets to the finalizeBundle point though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109052