This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add invalid match case for uimm2, uimm3 and uimm7
ClosedPublic

Authored by Jim on Sep 22 2021, 11:20 PM.

Diff Detail

Event Timeline

Jim created this revision.Sep 22 2021, 11:20 PM
Jim requested review of this revision.Sep 22 2021, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2021, 11:20 PM

Can this be tested?

xgupta added a subscriber: xgupta.Sep 23 2021, 2:35 AM
Jim updated this revision to Diff 374503.Sep 23 2021, 4:15 AM

Add testcases and fix incorrect operand of inst alias for InsnR4

Ping?

Please don't ping unless it's been about a week with no response. An email gets sent to the reviewers on any update and any update moves it to the top of our review queue on the website. So pinging again in the same 24 hours is kind of rude.

This revision is now accepted and ready to land.Sep 23 2021, 6:44 PM
Jim added a comment.Sep 23 2021, 7:55 PM

Ping?

Please don't ping unless it's been about a week with no response. An email gets sent to the reviewers on any update and any update moves it to the top of our review queue on the website. So pinging again in the same 24 hours is kind of rude.

Sorry about that. I would be careful next time.

Jim updated this revision to Diff 374714.Sep 23 2021, 7:59 PM

Rename funct7 to funct2.

Jim updated this revision to Diff 374727.EditedSep 23 2021, 9:57 PM

Add missing unrenamed operand.

jrtc27 requested changes to this revision.Sep 23 2021, 9:59 PM
jrtc27 added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfo.td
923 ↗(On Diff #374727)

You had a LGTM and now you're breaking thing by uploading new revisions that change this? R-type instructions have a funct7 field, not a funct2 field. I don't understand what you're doing here.

This revision now requires changes to proceed.Sep 23 2021, 9:59 PM
jrtc27 added inline comments.Sep 23 2021, 10:03 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.td
923 ↗(On Diff #374727)

Oh, this is the instruction definition for supporting bogus .insn r for R4 formats. Yes this should be uimm2. But I don't like the approach of tacking this onto a patch you already got a LGTM, especially when it's a different fix to what the patch subject says it's doing. At least amend the existing revision and make it clear you're expanding the scope compared with what you first put up for review...

Jim updated this revision to Diff 374730.Sep 23 2021, 10:49 PM

Move the change for incorrect operand type to D110381

Jim marked 2 inline comments as done.Sep 23 2021, 10:51 PM

I have created new revision D110381 for this.

Jim added a comment.Sep 28 2021, 6:24 PM

Hi @jrtc27,

Does it look good for you? Thanks for your review.

Jim added a comment.Oct 12 2021, 10:25 PM

Ping? Thanks.

craig.topper added inline comments.Oct 14 2021, 9:07 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1097

Can you put this between UImm5 and SImm5 so that similar code is together. I think that's better than keeping UImm5 and SImm5 next to each other.

Jim updated this revision to Diff 379906.Oct 14 2021, 8:29 PM

Address @craig.topper's comment.

Jim marked an inline comment as done.Oct 14 2021, 8:31 PM
craig.topper accepted this revision.Oct 14 2021, 8:42 PM

LGTM. I don't think we need @jrtc27 to review this.

jrtc27 accepted this revision.Oct 14 2021, 8:44 PM
This revision is now accepted and ready to land.Oct 14 2021, 8:44 PM

You can have a review anyway :)

You can have a review anyway :)

Thanks!

This revision was landed with ongoing or failed builds.Oct 15 2021, 12:20 AM
This revision was automatically updated to reflect the committed changes.