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

[Michael Liao via Phabricator via llvm-commits]

hliao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:1260
 
+  SmallPtrSet<MachineInstr *, 16> Folded;
   for (FoldCandidate &Fold : FoldList) {
----------------
arsenm wrote:
> 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
Thanks for the code review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93174