Page MenuHomePhabricator

[IR] Value: Fix OpCode checks
ClosedPublic

Authored by samitolvanen on Jun 10 2021, 9:29 AM.

Details

Summary

Value::SubclassID cannot be directly compared to Instruction enums, such as
Instruction::{Call,Invoke,CallBr}. We have to first subtract InstructionVal
from the SubclassID to get the OpCode, similar to Instruction::getOpCode().

Diff Detail

Event Timeline

samitolvanen created this revision.Jun 10 2021, 9:29 AM
samitolvanen requested review of this revision.Jun 10 2021, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2021, 9:29 AM
nickdesaulniers accepted this revision.Jun 10 2021, 10:13 AM

Based on the comments above Value::getValueID, this seems fine to me. ie. no such opcode == 0 and InstructionVal enumerator must be the highest valued enumerator in the ValueTy enum.

This revision is now accepted and ready to land.Jun 10 2021, 10:13 AM

Thanks, Nick. I don't have commit access, so would you mind committing the patch? I confirmed that check-all passes for me with the patch applied.

craig.topper added inline comments.
llvm/lib/IR/Value.cpp
67

Can we change this to CallBase while we're here? Only one of those opcodes is CallInst, the others are InvokeInst and CallBrInst. They all inherit from CallBase.

I was hoping there was a better constructor on CallBase to stash this in to simplify things, but I don't think there is.

Changed the message to refer to CallBase instead of CallInst.

samitolvanen marked an inline comment as done.Jun 10 2021, 12:10 PM
nickdesaulniers requested changes to this revision.Jun 10 2021, 12:11 PM
nickdesaulniers added inline comments.
llvm/lib/IR/Value.cpp
67

I think there's a 4th class derived from CallBase: GCStatePointInst, though there's nothing in langref about it; seems like an intrinsic. Probably should be fine. The text of the assert can be cleaned up, too.

This revision now requires changes to proceed.Jun 10 2021, 12:11 PM
nickdesaulniers accepted this revision.Jun 10 2021, 12:14 PM
This revision is now accepted and ready to land.Jun 10 2021, 12:14 PM
This revision was landed with ongoing or failed builds.Jun 10 2021, 4:46 PM
This revision was automatically updated to reflect the committed changes.