This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix crash in SIOptimizeExecMaskingPreRA
ClosedPublic

Authored by foad on Mar 30 2022, 8:35 AM.

Details

Summary

When folding a COPY of exec into another COPY, the call to
TII->isOperandLegal would crash because COPYs don't have defined
register classes for their operands.

Diff Detail

Event Timeline

foad created this revision.Mar 30 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:35 AM
foad requested review of this revision.Mar 30 2022, 8:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2022, 8:35 AM

Could also use a MIR test

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp
425 ↗(On Diff #419162)

A COPY isn't the only case where this happens though

427 ↗(On Diff #419162)

Newline is redundant here

foad added inline comments.Mar 30 2022, 8:49 AM
llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp
425 ↗(On Diff #419162)

I was wondering about that. How do all the existing callers of isOperandLegal manage not to hit this assertion?

llc: lib/Target/AMDGPU/SIInstrInfo.cpp:5013: bool llvm::SIInstrInfo::isOperandLegal(const llvm::MachineInstr &, unsigned int, const llvm::MachineOperand *) const: Assertion `DefinedRC' failed.
alex-t added inline comments.Apr 1 2022, 11:02 AM
llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp
425 ↗(On Diff #419162)

In fact, the operand always legal in COPY. So, SIInstrInfo::isOperandLegal may just return true for the case where no defined RC and the instruction opcode is COPY

alex-t accepted this revision.Apr 1 2022, 11:03 AM

LGTM if we do not want to change the COPY processing in the SIInstrInfo::isOperandLegal

This revision is now accepted and ready to land.Apr 1 2022, 11:03 AM
foad updated this revision to Diff 422797.Apr 14 2022, 3:04 AM

Handle any instruction without a defined RC, not just COPY.

foad added a comment.Apr 14 2022, 3:06 AM

LGTM if we do not want to change the COPY processing in the SIInstrInfo::isOperandLegal

How about this? As @arsenm pointed out this can happen for other instructions, not just COPY, so I handle them all the same way.

arsenm accepted this revision.Apr 20 2022, 6:23 AM
This revision was landed with ongoing or failed builds.Apr 20 2022, 6:45 AM
This revision was automatically updated to reflect the committed changes.