This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Expand ADD/SUBCARRY
ClosedPublic

Authored by leonardchan on May 1 2019, 4:14 PM.

Diff Detail

Event Timeline

leonardchan created this revision.May 1 2019, 4:14 PM
efriedma added inline comments.May 1 2019, 4:31 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3300

Are you sure this is right? The comment is correct, but there's only an overflow if the carry is set and the result is zero.

leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3300

My bad, you're right. Updated.

efriedma accepted this revision.May 1 2019, 5:28 PM

LGTM

Please run some basic sanity tests before you merge to make sure this works correctly at runtime. The code looks right, but I don't really trust that. :) If you want to test something simpler than smul.fix, might make sense to just temporarily hack legalization for addition to use this codepath, or something like that, so you can easily understand what you're actually testing.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3300

Looks okay now.

This revision is now accepted and ready to land.May 1 2019, 5:28 PM

LGTM

Please run some basic sanity tests before you merge to make sure this works correctly at runtime. The code looks right, but I don't really trust that. :) If you want to test something simpler than smul.fix, might make sense to just temporarily hack legalization for addition to use this codepath, or something like that, so you can easily understand what you're actually testing.

Good thing I checked! I did as you suggested by forcing 32bit x86 to take this path and found a bunch of type mismatches. After fixing that, everything looks good for expected results. Will submit tomorrow morning if there aren't any more comments then.

jyknight added inline comments.May 2 2019, 7:30 AM
llvm/test/CodeGen/SPARC/addcarry.ll
2 ↗(On Diff #197702)

SPARC is probably not a great example, because it can support ADDCARRY fine, only nobody implemented it yet (just the deprecated ADDC/ADDE). RISCV would be a better test-case, because it actually doesn't have an overflow flag at all.

efriedma added inline comments.May 2 2019, 10:44 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3292

Looking at this again, it isn't quite right. In general, on targets where getBooleanContents does not return ZeroOrOneBooleanContent, you need to getZeroExtendInReg to convert a carry bit to 1/0.

leonardchan marked an inline comment as done.
leonardchan marked an inline comment as done.May 2 2019, 12:12 PM
leonardchan added inline comments.
llvm/test/CodeGen/SPARC/addcarry.ll
2 ↗(On Diff #197702)

So I'm having trouble figuring out how to properly test this since RISCV also doesn't seem to support the MUL_LOHI ISDs and I don't know other ways to invoke usage of ISD::ADDCARRY other than through expandMUL_LOHI.

Do you think it would be fine if I also added an intrinsic like llvm.addcarry for explicitly expanding to ISD::ADDCARRY so I could test this directly?

efriedma added inline comments.May 2 2019, 12:18 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3292

I think you need both the getZExtOrTrunc to fix the type, and the getZeroExtendInReg to fix the value. (getZeroExtendInReg just produces an ISD::AND.)

efriedma added inline comments.May 2 2019, 1:08 PM
llvm/test/CodeGen/SPARC/addcarry.ll
2 ↗(On Diff #197702)

Try "llc -mtriple=riscv32 -mattr=+m"?

leonardchan marked 4 inline comments as done.May 2 2019, 2:01 PM
leonardchan added inline comments.
llvm/test/CodeGen/SPARC/addcarry.ll
2 ↗(On Diff #197702)

This works well. Didn't know about this beforehand. Thanks.

efriedma added inline comments.May 2 2019, 3:19 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3293

The VT argument for getZeroExtendInReg is the mask, so should be MVT::i1. The way it's written, the getZeroExtendInReg does nothing.

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3293

Updated

efriedma accepted this revision.May 2 2019, 4:19 PM

LGTM with a minor comment.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3312

Shouldn't matter in practice, but the second type passed to getBoolExtOrTrunc is supposed to be the type of the operation ("VT").

This revision was automatically updated to reflect the committed changes.
leonardchan marked an inline comment as done.