This is an archive of the discontinued LLVM Phabricator instance.

Don't fold double constant to an integer if dest type not integral
ClosedPublic

Authored by tejohnson on Mar 28 2016, 2:17 PM.

Details

Summary

I encountered this issue when constant folding during inlining tried to
fold away a bitcast of a double to an x86_mmx, which is not an integral
type. The test case exposes the same issue with a smaller code snippet
during early CSE.

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 51842.Mar 28 2016, 2:17 PM
tejohnson retitled this revision from to Don't fold double constant to an integer if dest type not integral.
tejohnson updated this object.
tejohnson added a subscriber: llvm-commits.
ab added a subscriber: ab.Mar 29 2016, 9:37 AM

x86_mmx strikes again!

LGTM, but the test shouldn't be in CodeGen. It's not great either, but what about Transforms/EarlyCSE?

test/CodeGen/X86/mmx-bitcast-fold.ll
2

Perhaps x86_64-- ?

6

-internal

9

-tail

Thanks for the review!

LGTM, but the test shouldn't be in CodeGen. It's not great either, but what about Transforms/EarlyCSE?

The bug isn't specific to EarlyCSE either (I originally found this when folding was invoked from the inliner). I wasn't sure where to put this but there are already a bunch of X86 constant folding tests in CodeGen/X86. It seemed consistent with past tests to put it there. Ok if I leave it there?

test/CodeGen/X86/mmx-bitcast-fold.ll
2

Good idea.

6

done

9

done

tejohnson updated this revision to Diff 51941.Mar 29 2016, 9:49 AM
  • Address review comments.
ab added a comment.Mar 30 2016, 2:52 PM

Thanks for the review!

LGTM, but the test shouldn't be in CodeGen. It's not great either, but what about Transforms/EarlyCSE?

The bug isn't specific to EarlyCSE either (I originally found this when folding was invoked from the inliner). I wasn't sure where to put this but there are already a bunch of X86 constant folding tests in CodeGen/X86. It seemed consistent with past tests to put it there. Ok if I leave it there?

Yeah, I know :/
Looking at the history of ConstantFold.cpp, most changes seem to add a test to the relevant transform directory. The closest to dedicated tests seem to be Assembler/ConstantExpr*Fold*.ll.

tejohnson updated this revision to Diff 52201.Mar 31 2016, 7:14 AM

Move new test to test/Assembler where there are some existing constant
fold tests.

In D18528#387557, @ab wrote:

Yeah, I know :/
Looking at the history of ConstantFold.cpp, most changes seem to add a test to the relevant transform directory. The closest to dedicated tests seem to be Assembler/ConstantExpr*Fold*.ll.

Moved it to test/Assembler. Let me know if it looks ok now. Thanks!

Ping

Thanks,
Teresa

ab accepted this revision.Apr 4 2016, 2:12 PM
ab added a reviewer: ab.

Eh, it's even more out of place there. I thought it was possible to tickle it with:

@mmx = global x86_mmx bitcast (double 0.000000e+00 to x86_mmx)

but that's rejected.

Anyway, sorry about the pointless back and forth; LGTM in Transforms or CodeGen, your pick.

This revision is now accepted and ready to land.Apr 4 2016, 2:12 PM
In D18528#391693, @ab wrote:

Eh, it's even more out of place there. I thought it was possible to tickle it with:

@mmx = global x86_mmx bitcast (double 0.000000e+00 to x86_mmx)

but that's rejected.

Anyway, sorry about the pointless back and forth; LGTM in Transforms or CodeGen, your pick.

Ok, thanks. I'm going to move it back to CodeGen/X86 since that's where I see similar tests.

This revision was automatically updated to reflect the committed changes.