This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Specialize mixed-sign mul-with-overflow (fix PR34920)
ClosedPublic

Authored by vsk on Dec 12 2017, 5:54 PM.

Details

Summary

This patch introduces a specialized way to lower overflow-checked
multiplications with mixed-sign operands. This fixes link failures and
ICEs on code like this:

void mul(int64_t a, uint64_t b) {
  int64_t res;
  __builtin_mul_overflow(a, b, &res);
}

The generic checked-binop irgen would use a 65-bit multiplication
intrinsic here, which requires runtime support for _muloti4 (128-bit
multiplication), and therefore fails to link on i386. To get an ICE
on x86_64, change the example to use int128_t / uint128_t.

Adding runtime and backend support for 65-bit or 129-bit checked
multiplication on all of our supported targets is infeasible.

This patch solves the problem by using simpler, specialized irgen for
the mixed-sign case.

llvm.org/PR34920, rdar://34963321

Testing: Apart from check-clang, I compared the output from this fairly
comprehensive test driver using unpatched & patched clangs:
https://gist.github.com/vedantk/3eb9c88f82e5c32f2e590555b4af5081

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Dec 12 2017, 5:54 PM
vsk updated this revision to Diff 126666.Dec 12 2017, 6:08 PM
  • Make sure the result can be stored into the result ptr.
efriedma added inline comments.Dec 13 2017, 12:34 PM
test/CodeGen/builtins-overflow.c
402 ↗(On Diff #126666)

I think the rules for __builtin_mul_overflow say you have to check whether the truncate changes the value of the result.

Missing testcases for the case where the result is unsigned.

vsk updated this revision to Diff 126854.Dec 13 2017, 3:32 PM
vsk marked an inline comment as done.
vsk edited the summary of this revision. (Show Details)
test/CodeGen/builtins-overflow.c
402 ↗(On Diff #126666)

Thanks for the catch. I've updated my test driver to catch cases like this.

vsk edited the summary of this revision. (Show Details)Dec 13 2017, 3:37 PM
efriedma accepted this revision.Dec 15 2017, 5:19 PM

LGTM

lib/CodeGen/CGBuiltin.cpp
912 ↗(On Diff #126854)

zext() rather than zextOrSelf().

This revision is now accepted and ready to land.Dec 15 2017, 5:19 PM
vsk marked an inline comment as done.Dec 15 2017, 5:21 PM

Thanks for the review!

lib/CodeGen/CGBuiltin.cpp
912 ↗(On Diff #126854)

Will fix before committing.

This revision was automatically updated to reflect the committed changes.