This is an archive of the discontinued LLVM Phabricator instance.

Fix icmp lowering
ClosedPublic

Authored by chfast on Apr 21 2015, 6:12 AM.

Details

Summary

During icmp lowering it can happen that a constant value can be larger than expected (see the code around the change).
APInt::getMinSignedBits() must be checked again as the shift before can change the constant sign to positive.
I'm not sure it is the best fix possible though.

Diff Detail

Repository
rL LLVM

Event Timeline

chfast updated this revision to Diff 24120.Apr 21 2015, 6:12 AM
chfast retitled this revision from to Fix icmp lowering.
chfast updated this object.
chfast edited the test plan for this revision. (Show Details)
chfast added a reviewer: resistor.
chfast added a subscriber: Unknown Object (MLST).
chfast set the repository for this revision to rL LLVM.
hfinkel added inline comments.
lib/CodeGen/SelectionDAG/TargetLowering.cpp
1727 ↗(On Diff #24120)

Why 64? Shouldn't this depend on the size of the type?

test/CodeGen/Generic/icmp-illegal.ll
4 ↗(On Diff #24120)

What is this test testing? Did these crash previously?

chfast added inline comments.May 19 2015, 5:09 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
1727 ↗(On Diff #24120)

This check is needed to later NewC.getSExtValue() be always correct. isLegalICmpImmediate checks the real value.

test/CodeGen/Generic/icmp-illegal.ll
4 ↗(On Diff #24120)

Previously that caused an assert failure in NewC.getSExtValue().

hfinkel added inline comments.May 20 2015, 9:27 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
1727 ↗(On Diff #24120)

Sure, but why 64?

test/CodeGen/Generic/icmp-illegal.ll
4 ↗(On Diff #24120)

Okay, thanks.

chfast added inline comments.May 20 2015, 9:30 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
1727 ↗(On Diff #24120)

If it less than 64 you can fit the value in int64_t. If not you cannot. It's all about C++ types, not LLVM types.

hfinkel accepted this revision.May 20 2015, 9:35 AM
hfinkel added a reviewer: hfinkel.

LGTM.

lib/CodeGen/SelectionDAG/TargetLowering.cpp
1727 ↗(On Diff #24120)

Okay, thanks. It is that NewC.getSExtValue() gives the int64_t.

This revision is now accepted and ready to land.May 20 2015, 9:35 AM
This revision was automatically updated to reflect the committed changes.