Page MenuHomePhabricator

CodeGen: Improve generated IR for __builtin_mul_overflow(uint, uint, int)
AcceptedPublic

Authored by tstellar on Jul 23 2020, 7:16 AM.

Details

Reviewers
vsk
efriedma
Summary

Add a special case for handling builtin_mul_overflow with unsigned
inputs and a signed output to avoid emitting the
muloti4 library
call on x86_64. muloti4 is not implemented in libgcc, so avoiding
this call fixes compilation of some programs that call
builtin_mul_overflow with these arguments.

For example, this fixes the build of cpio with clang, which includes code from
gnulib that calls __builtin_mul_overflow with these argument types.

Diff Detail

Event Timeline

tstellar created this revision.Jul 23 2020, 7:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2020, 7:16 AM
tstellar updated this revision to Diff 280104.Jul 23 2020, 7:18 AM

Remove stray comment.

vsk added a comment.Jul 23 2020, 10:26 AM

How were you able to show that the specialized IRGen is equivalent to the generic kind? I tried doing this with direct inspection (https://godbolt.org/z/o5WEn3), but wasn't able to convince myself. In the past I used a test driver to compare the before/after compiler output -- you might find that useful (https://gist.github.com/vedantk/3eb9c88f82e5c32f2e590555b4af5081).

clang/test/CodeGen/builtins-overflow.c
123

Could you add a test for the volatile result case?

In D84405#2170110, @vsk wrote:

How were you able to show that the specialized IRGen is equivalent to the generic kind? I tried doing this with direct inspection (https://godbolt.org/z/o5WEn3), but wasn't able to convince myself. In the past I used a test driver to compare the before/after compiler output -- you might find that useful (https://gist.github.com/vedantk/3eb9c88f82e5c32f2e590555b4af5081).

I compared with what gcc was generating and it looked similar (although my x86 assembly knowledge is not great), but I will give your test driver a try, thanks.

tstellar updated this revision to Diff 281092.Jul 27 2020, 6:05 PM

Add 64-bit test.

This is the test driver I used for testing. I compared the clang 11 + this patch with gcc 10 and saw no differences: https://gist.github.com/tstellar/80dae2ab8a18d810b10b8e42777f4fe4

Here is the assembly comparison for the 64-bit test I added between gcc and clang trunk + patch: https://reviews.llvm.org/P8227

vsk accepted this revision.Jul 28 2020, 12:10 PM

Thanks, looks good to me with a small test addition.

clang/test/CodeGen/builtins-overflow.c
123

I think this is still missing?

This revision is now accepted and ready to land.Jul 28 2020, 12:10 PM
tstellar updated this revision to Diff 281419.Jul 28 2020, 4:47 PM

Add volatile test.

tstellar marked an inline comment as done.Jul 28 2020, 4:48 PM
tstellar added inline comments.
clang/test/CodeGen/builtins-overflow.c
123

Yes, I forgot to add this, there is one in the latest patch now.