This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Optimize {s|u}mul.with.overflow.
ClosedPublic

Authored by jgalenson on Dec 6 2017, 2:54 PM.

Details

Summary

This extends my previous patches to also optimize overflow-checked multiplies during SelectionDAG.

Diff Detail

Event Timeline

jgalenson created this revision.Dec 6 2017, 2:54 PM
rogfer01 added inline comments.Dec 8 2017, 10:35 AM
lib/Target/ARM/ARMISelLowering.cpp
3955

What about

// We generate a SMUL_LOHI and then check if all the bits of the high word
// are the same as the sign bit of the low word.
4479

I think this comment should be updated after this change.

4527

I think this comment should be updated after this change.

test/CodeGen/ARM/su-addsub-overflow.ll
80

Looks like we are overloading a bit the name of this test file. Perhaps add a new testfile su-mul-overflow.ll instead?

jgalenson added inline comments.Dec 8 2017, 11:37 AM
lib/Target/ARM/ARMISelLowering.cpp
3955

That's definitely better. Thanks!

4479

Good catch!

test/CodeGen/ARM/su-addsub-overflow.ll
80

Good point. I think I prefer having them all in the same file since they're testing the same sort of things, so I'll rename the file. But I can certainly split it if you think that's best.

jgalenson updated this revision to Diff 126188.Dec 8 2017, 11:37 AM

Update comments and rename test file.

rogfer01 accepted this revision.Jan 10 2018, 10:48 AM

LGTM. Please wait a couple of days before submitting just in case @efriedma has further comments.

This revision is now accepted and ready to land.Jan 10 2018, 10:48 AM
This revision was automatically updated to reflect the committed changes.

Thanks @alexcrichton . I cannot login to bugzilla yet (I'd reply there instead, hope I can soon). Looks like {S,U}MUL_LOHI are expanded in Thumb1 so a stop-gap approach could be not implementing this optimisation in that subtarget.

Sorry for the trouble. I see it's been fixed now.

Oh no worries at all, thanks!