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.

Download Raw Diff

Details

Reviewers

ab
majnemer

Commits

rGf4cf1c3eb493: Don't fold double constant to an integer if dest type not integral
rL265367: Don't fold double constant to an integer if dest type not integral

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

Repository: rL LLVM

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.

tejohnson added a reviewer: majnemer.Mar 29 2016, 8:26 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
1 ↗	(On Diff #51842)	Perhaps x86_64-- ?
5 ↗	(On Diff #51842)	-internal
8 ↗	(On Diff #51842)	-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
1 ↗	(On Diff #51842)	Good idea.
5 ↗	(On Diff #51842)	done
8 ↗	(On Diff #51842)	done

Address review comments.

In D18528#385777, @tejohnson wrote:

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.

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

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.

Closed by commit rL265367: Don't fold double constant to an integer if dest type not integral (authored by tejohnson). · Explain WhyApr 4 2016, 4:56 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

llvm/

trunk/

lib/

IR/

ConstantFold.cpp

4 lines

test/

CodeGen/

X86/

mmx-bitcast-fold.ll

12 lines

Diff 52639

llvm/trunk/lib/IR/ConstantFold.cpp

Show First 20 Lines • Show All 185 Lines • ▼ Show 20 Lines	if (ConstantFP *FP = dyn_cast<ConstantFP>(V)) {
// double is always stored first in memory, regardless of the target		// double is always stored first in memory, regardless of the target
// endianness. The memory layout of i128, however, depends on the target		// endianness. The memory layout of i128, however, depends on the target
// endianness, and so we can't fold this without target endianness		// endianness, and so we can't fold this without target endianness
// information. This should instead be handled by		// information. This should instead be handled by
// Analysis/ConstantFolding.cpp		// Analysis/ConstantFolding.cpp
if (FP->getType()->isPPC_FP128Ty())		if (FP->getType()->isPPC_FP128Ty())
return nullptr;		return nullptr;

		// Make sure dest type is compatible with the folded integer constant.
		if (!DestTy->isIntegerTy())
		return nullptr;

return ConstantInt::get(FP->getContext(),		return ConstantInt::get(FP->getContext(),
FP->getValueAPF().bitcastToAPInt());		FP->getValueAPF().bitcastToAPInt());
}		}

return nullptr;		return nullptr;
}		}


▲ Show 20 Lines • Show All 2,067 Lines • Show Last 20 Lines

llvm/trunk/test/CodeGen/X86/mmx-bitcast-fold.ll

				; RUN: opt -mtriple=x86_64-- -early-cse < %s -S \| FileCheck %s

				; CHECK: @foo(x86_mmx bitcast (double 0.000000e+00 to x86_mmx))

				define void @bar() {
				entry:
				%0 = bitcast double 0.0 to x86_mmx
				%1 = call x86_mmx @foo(x86_mmx %0)
				ret void
				}

				declare x86_mmx @foo(x86_mmx)