[PATCH] D93174: [amdgpu] Fix a crash case when `V_CNDMASK` could be simplified.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:1260
 
+  SmallPtrSet<MachineInstr *, 16> Folded;
   for (FoldCandidate &Fold : FoldList) {
----------------
hliao wrote:
> arsenm wrote:
> > hliao wrote:
> > > arsenm wrote:
> > > > Doesn't this add a second mechanism to avoid the same problem? We already check isUseMIInFoldList to avoid revisiting
> > > That one is used to prevent adding a commuted instr into the candidate list. But the case here is that both operands could be folded. Also, if used here, that check is too expensive? That candidate list will be scanned in square times.
> > Should we just use a SetVector for FoldList then?
> That list is a list of operand foldable, i.e., the pair MI and one of its operand being folded. If we change that to be keyed by MI, besides major data structure change, we may add extra overhead (set vs list) when building that candidate list. Considering that `tryFoldInst` only simplifies `V_CNDMASK` so far, is that overhead too big to justify the skip list here?
Hmmm. In principle tryFoldInst would erase / replace the instruction, but it just happens to not. I do think the way this pass works is backwards, but I guess this is fine for now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93174