This is an archive of the discontinued LLVM Phabricator instance.

[amdgpu] Fix a crash case when `V_CNDMASK` could be simplified.
ClosedPublic

Authored by hliao on Dec 12 2020, 9:41 PM.

Details

Summary
  • Once an instruction is simplified, foldable candidates from it should be invalidated or skipped as the operand index is no longer valid.

Diff Detail

Unit TestsFailed

Event Timeline

hliao created this revision.Dec 12 2020, 9:41 PM
hliao requested review of this revision.Dec 12 2020, 9:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2020, 9:41 PM
arsenm added inline comments.Dec 14 2020, 6:29 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

Doesn't this add a second mechanism to avoid the same problem? We already check isUseMIInFoldList to avoid revisiting

hliao added inline comments.Dec 14 2020, 6:58 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

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.

arsenm added inline comments.Dec 14 2020, 7:24 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

Should we just use a SetVector for FoldList then?

hliao added inline comments.Dec 14 2020, 8:11 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

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?

arsenm accepted this revision.Dec 14 2020, 9:44 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

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

This revision is now accepted and ready to land.Dec 14 2020, 9:44 AM
This revision was landed with ongoing or failed builds.Dec 14 2020, 10:08 AM
This revision was automatically updated to reflect the committed changes.
hliao added inline comments.Dec 14 2020, 10:10 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
1260

Thanks for the code review