This is an archive of the discontinued LLVM Phabricator instance.

Fix backward operands in call to isTruncateFree() and improve comments.
ClosedPublic

Authored by srking on Mar 12 2015, 12:13 PM.

Details

Reviewers
resistor
hfinkel
Summary

In at least one case, DAGCombiner calls isTruncateFree() with operands in reverse order. Fixed that and improved comments to make the direction and corner case behavior more obvious.

Diff Detail

Event Timeline

srking updated this revision to Diff 21866.Mar 12 2015, 12:13 PM
srking retitled this revision from to Fix backward operands in call to isTruncateFree() and add asserts..
srking updated this object.
srking edited the test plan for this revision. (Show Details)
srking added a subscriber: Unknown Object (MLST).

Oops, patch is in wrong direction. Will upload new patch.

srking updated this revision to Diff 21869.Mar 12 2015, 12:57 PM

Fixed patch file direction.

srking updated this revision to Diff 21887.Mar 12 2015, 4:28 PM

Removed the assert() due to cases that call isTruncateFree without regard to type size. Improved comment for these odd cases.
Verified regression checks pass.

arsenm added a subscriber: arsenm.Jul 28 2015, 10:20 AM

Do you have a testcase for this?

jroelofs added inline comments.
include/llvm/Target/TargetLowering.h
1562–1566

Please fix this one...

1570

... and this one too while you're here.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9722–9723

Drop this comment, and don't change the formatting of the following lines.

jroelofs added inline comments.Jul 28 2015, 10:22 AM
include/llvm/Target/TargetLowering.h
1519–1520

Would be nice to fix this one too.

Hi,

The fix LGTM, but do you have a test case exposing the problem?

Thanks,
-Quentin

srking updated this revision to Diff 30840.Jul 28 2015, 11:52 AM
srking retitled this revision from Fix backward operands in call to isTruncateFree() and add asserts. to Fix backward operands in call to isTruncateFree() and improve comments..
srking updated this object.

Tried to address review comments from Jonathan Roelofs. NFC from previous patch. Updated patch title text and summary to remove statement about asserts.

thanks!

include/llvm/Target/TargetLowering.h
1557–1558

s/Ty2/FromTy/ s/Ty1/ToTy/

1561–1562

s/Ty2/FromTy/ s/Ty1/ToTy/

srking marked 4 inline comments as done.Jul 28 2015, 12:01 PM

Do you have a testcase for this?

Unfortunately, I do not see a succinct way to test for this. The bug is buried in a cost analysis algorithm. The problem was detected by investigating nonsense truncation cases, i.e. where From < To. By inspection, we see the original code places the destination as "From" and the operand as "To" in the truncation check.

I verified all "make check" tests pass with the patch.

Thanks,
-steve

srking added inline comments.Jul 28 2015, 12:33 PM
include/llvm/Target/TargetLowering.h
1557–1558

rather s/Ty1/FromTy/ s/Ty2/ToTy/

1561–1562

I understand x86 behavior here, but I can't understand what the original comment is trying to say. Taking your word for it.

srking updated this revision to Diff 30845.Jul 28 2015, 12:38 PM

Fix more old style comments caught by Jonathan. NFC.
The meaning of one of the original comments is mysterious. This patch does not clarify it.

jroelofs added inline comments.Jul 28 2015, 12:52 PM
include/llvm/Target/TargetLowering.h
1561–1562

Now that I've read it again, I don't get what it's trying to say either... I can kind of see it making sense if the operands were accidentally swapped.

srking marked 3 inline comments as done.Jul 28 2015, 1:39 PM
srking added inline comments.
include/llvm/Target/TargetLowering.h
1561–1562

Ya, I tried for a while to assimilate this one. Confusing comments are often worse than nothing. If you aprove, I will update the patch to delete this comment.

hfinkel accepted this revision.Aug 9 2015, 11:02 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

This LGTM. If you have a test case, great, otherwise you can commit without one.

include/llvm/Target/TargetLowering.h
1561–1562

Exactly what would you want to delete? Perhaps we should just reword this to say that the function should return true when it is likely that the truncate can be freely folded with an instruction defining a value of FromTy. If the defining instruction is unknown (because you're looking at a function argument, PHI, etc.) then you may need an explicit truncate, and that's not necessarily free (but this function does not deal with those cases).

This revision is now accepted and ready to land.Aug 9 2015, 11:02 PM
srking updated this revision to Diff 32069.Aug 13 2015, 9:26 AM
srking edited edge metadata.

Updated with improved comment from Hal Finkel.

srking closed this revision.Aug 19 2015, 11:43 AM
srking marked an inline comment as done.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@245385 91177308-0d34-0410-b5e6-96231b3b80d8