Page MenuHomePhabricator

Add missing cases in RISCVMCExpr::getVariantKindName
ClosedPublic

Authored by sepavloff on Mar 19 2021, 1:59 AM.

Diff Detail

Event Timeline

sepavloff created this revision.Mar 19 2021, 1:59 AM
sepavloff requested review of this revision.Mar 19 2021, 1:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 1:59 AM
Herald added a subscriber: MaskRay. · View Herald Transcript

Is it possible to test this?

I think these are just for debug output? Though maybe they appear in MIR too?

Updated patch

  • changed switch statement so that adding new item to VariantKind would cause compilation warning if the respective case is not added.
  • updated RISCVMCExpr::getVariantKindForName as well.
craig.topper added inline comments.Mar 19 2021, 10:42 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
171

Can this just be llvm_unreachable and then you wouldn't need the first assert?

Is it possible to test this?

I do not know how to test this. But I changed the patch so that if a new item would be added to VariantKind and that item would not be handled in the switch statement, compilation would give a warning.

I think these are just for debug output? Though maybe they appear in MIR too?

I got crash when executed llc -debug -filetype=obj. I am not sure if it can be found in MIR, but this looks unlikely, as it is an MC layer feature.

Use llvm_unreachable instead of bogus return

Removed unneeded assert

Does this tiny patch require some additional changes?

This revision is now accepted and ready to land.Tue, Mar 23, 9:54 PM
craig.topper requested changes to this revision.Tue, Mar 23, 10:02 PM
craig.topper added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
136

Looking again. The only caller of this is RISCVAsmParser::parseOperandWithModifier. Are we allowing something additional in assembly parsing with this change? Is that something we should test?

This revision now requires changes to proceed.Tue, Mar 23, 10:02 PM
sepavloff updated this revision to Diff 332941.Wed, Mar 24, 4:30 AM

Remove changes in RISCVMCExpr::getVariantKindForName

craig.topper accepted this revision.Wed, Mar 24, 9:45 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Wed, Mar 24, 9:45 AM
This revision was automatically updated to reflect the committed changes.