This is an archive of the discontinued LLVM Phabricator instance.

Fix signed multiplication with overflow fallback.
ClosedPublic

Authored by jamesduley on Apr 7 2017, 6:04 AM.

Details

Summary

For targets that don't have ISD::MULHS or ISD::SMUL_LOHI for the type
and the double width type is illegal, then the two operands are
sign extended to twice their size then multiplied to check for overflow.
The extended upper halves were mismatched causing an incorrect result.
This fixes the mismatch.

A test was added for ARM V6-M where the bug was detected.

Original bug report here https://github.com/rust-lang/rust/issues/39056

Diff Detail

Repository
rL LLVM

Event Timeline

jamesduley created this revision.Apr 7 2017, 6:04 AM
ab accepted this revision.Apr 13 2017, 1:19 AM
ab added a subscriber: ab.

Nice catch, LGTM.

Looks like this should already be exercised by test/CodeGen/ARM/umulo-32.ll; I'd say remove that file entirely.

test/CodeGen/ARM/v6m-smul-with-overflow.ll
3 ↗(On Diff #94512)

Remove 'unnamed_addr' ?

This revision is now accepted and ready to land.Apr 13 2017, 1:19 AM

removed unnamed_addr

jamesduley marked an inline comment as done.Apr 13 2017, 10:19 AM

Looks like this should already be exercised by test/CodeGen/ARM/umulo-32.ll; I'd say remove that file entirely.

That's testing unsigned though, so shouldn't I leave it?

rs edited edge metadata.Apr 26 2017, 6:39 AM

Looks fine, I think it's ready to land. Do you have commit access ? If not do you want me to commit on your behalf ?

In D31807#738030, @rs wrote:

Looks fine, I think it's ready to land. Do you have commit access ? If not do you want me to commit on your behalf ?

No, I don't so yes please.

This revision was automatically updated to reflect the committed changes.