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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:3142-3146
+    while (Start->getOpcode() != FrameSetupOpcode)
+      --Start;
+    MachineBasicBlock::iterator End(&MI);
+    while (End->getOpcode() != FrameDestroyOpcode)
+      ++End;
----------------
sebastian-ne wrote:
> arsenm wrote:
> > sebastian-ne wrote:
> > > arsenm wrote:
> > > > I'm not sure you can guarantee this is the range that needs to be moved. Could other instructions have been moved across these between the IRTranslator and here?
> > > The running passes seem to be
> > > - IRTranslator
> > > - AMDGPUPreLegalizerCombiner
> > > - Localizer
> > > - Legalizer
> > > - AMDGPUPostLegalizerCombiner
> > > - RegBankSelect
> > > 
> > > It’s fine if VALU instructions are moved inside the loop, but SALU instructions would be a problem.
> > > 
> > > Can we prevent instructions from being moved into the call code?
> > I was thinking of doing this before by using virtual registers for the call pseudo and expanding to physical registers later, but it seems like overkill. Practically speaking we don't really have code reordering right now that would present an issue, and don't see why we would ever add it. The localizer does some level of reordering, but it's really just a simple sink to uses. In practice we'll only see copies inside this range. Overall I can't see a real issue doing it this way, but it still feels wrong since there's no formal guarantee
> I tried out putting the call code inside adjcallstackup to -down into a bundle when it is generated, but I hit some problems.
> 1. The MachineVerifier complains about undefined physical registers when they are defined and used inside the bundle. It only considers def-ed registers live after a bundle, not inside it, which is more the style of vliw-bundles.
> 2. Bundles in SSA form (before register allocation) apparently do not have a BUNDLE instruction and no defs/uses on the bundle. Also, a bundle always needs an instruction before it, so having a call as the first instruction of a block does not work, because there is no instruction before the bundle, which is rather unfortunate.
> Also, a bundle always needs an instruction before it

Really? Why?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109052