This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Enable usubo formation in CodeGenPrepare.
Changes PlannedPublic

Authored by craig.topper on Mar 24 2023, 11:33 AM.

Details

Summary

The default behavior of shouldFormOverflowOp is to only enable
for uaddo.

This patch enables it for consistency. We can work on other fixes to
improve the generated code.

Diff Detail

Event Timeline

craig.topper created this revision.Mar 24 2023, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 11:33 AM
craig.topper requested review of this revision.Mar 24 2023, 11:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 11:33 AM
reames added inline comments.Mar 24 2023, 11:41 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
499

Doesn't this also enable multiply as well? Do we want to?

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.h
499

You might think that from the name but its never called with ISD::UMULO. I can add an assert?

reames accepted this revision.Mar 24 2023, 1:32 PM

LGTM w/comment

llvm/lib/Target/RISCV/RISCVISelLowering.h
496

Should this maybe be checking that VT is legal? Forming a 64 bit op on RV32 or a 32 bit op on RV64 seems slightly suspect.

499

Yes please.

This revision is now accepted and ready to land.Mar 24 2023, 1:32 PM
asb added a comment.Mar 24 2023, 1:37 PM

I start writing a comment grumbling that this adds undesirably tight coupling between with the implementation details of TargetLowering::shouldFormOverflowOp, but when I got to the end of it figured it wasn't that bad. If the comment had just said "Allow the transform whenever it would be allowed for UADDO" I might not have commented. LGTM. Though an assert like you suggest probably makes sense.

craig.topper added inline comments.Mar 24 2023, 1:51 PM
llvm/lib/Target/RISCV/RISCVISelLowering.h
496

We seem to do ok with W instructions for i32 on RV64.

For i64 on RV32, there was a proposed patch that disabled it, but then required other patches. I applied a couple patches to type legalization for uaddo and add that fixed the motivating case. I haven't seen evidence of other problems yet.

liaolucy added inline comments.Mar 25 2023, 2:59 AM
llvm/test/CodeGen/RISCV/overflow-intrinsics.ll
781

This regression cannot to be solved by adding a special case.

It seems we can modify ExpandIntRes_UADDSUBO by referring to ExpandIntRes_ADDSUB, I'm not sure.

craig.topper planned changes to this revision.May 1 2023, 12:05 PM

Moving to Plan Changes until I circle back to look at the regressions.