[PATCH] D131246: [AMDGPU] SIFixSGPRCopies reworking to use one pass over the MIR for analysis and lowering.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp:1108
+  if (T != PostProcessingLists.end()) {
+    T->second.List->erase(T->second.I);
+    PostProcessingLists.erase(MI);
----------------
alex-t wrote:
> rampitec wrote:
> > Why not simply clean all of these lists/vectors/maps at the end of the runOnMachineFunction?
> The reason for that change is as follows:
> We cannot lower SCC copies, REG_SEQUENCE, and PHIs **//before//** VGPR to SGPR copies since any lowering could change the MIR and, hence, make the analysis results invalid.
> On other hand, in VGPR to SGPR copies lowering "SIInstrInfo::moveToVALU & Co" may be invoked. They remove MIs from the basic block and poison the memory allocated for them. So, all the pointers stored in the lists can potentially point to the poisoned memory and MI->getParent() check becomes invalid. This is the exact reason for the ASAN failure.
> The "OnDeleteListener" interface serves as the delegate passed to "SIInstrInfo::moveToVALU" to make the caller informed of the MI->eraseFromParent() calls.
I don't think this complicated list management is better than a separate loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131246