This is an archive of the discontinued LLVM Phabricator instance.

patch for missed constantFold optimization in InstCombine
Needs ReviewPublic

Authored by optimusPrime on Nov 16 2015, 11:52 AM.

Details

Summary

llvm::ConstantFoldCompareInstOperands misses an optimization when it tries to constantFold a compare instruction containing addrspacecast instead of combination of ptrtoint and inttoptr. Adding the patched code snippet in the code would enable the pass to handle addrspacecast and constantFold the missing cases.
Attached is the reproducer. Let me know if there are any suggestions/comments.

Diff Detail

Event Timeline

optimusPrime retitled this revision from to patch for missed constantFold optimization in InstCombine.
optimusPrime updated this object.
optimusPrime added a reviewer: llvm-commits.
optimusPrime added a subscriber: llvm-commits.

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

Hi David,
I have already attached a test case in the summary section. Isn't that sufficient ?

This needs a test case. Also, you need to upload a diff with more context. There are instructions here on how to do this.

Hi David,
I have already attached a test case in the summary section. Isn't that sufficient ?

It should be part of the patch.

Uploaded the testcase scenario in the patch itself instead of summary.

Hi David,
Added the test case as a part of the patch as you suggested. Does this look okay ?

Thanks,
Tejas

Please include additional context in the diff.

test/Transforms/InstCombine/instCombineOptimizeAddrspaceCast.ll
9–27

Can you try to make this test a little more targeted?

Hi David,
Added some more context in both the .ll and cpp. I don't think the ll can be shortened more, this is the bare minimum needed to test.

Thanks

meadori added inline comments.
lib/Analysis/ConstantFolding.cpp
1140

Seems to be some extraneous spaces here.

1147

Is this patch against mainline trunk?

Invocations of ConstantFoldCompareInstOperands that I see look like:

ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);
mgrang added a subscriber: mgrang.Nov 20 2015, 11:53 AM
mgrang added inline comments.
lib/Analysis/ConstantFolding.cpp
1141

Ideally it's best to submit the entire file containing your changes. I would do this as follows:

git diff <previous_commit_id> HEAD -U99999 > my.patch

1142

Maximize the 80 column line width here.

1147

Your patch seems to be based on an older version of code. In the latest tip the signature of ConstantFoldCompareInstOperands does not use "TD". It uses DataLayout (DL) instead:

return ConstantFoldCompareInstOperands(Predicate, C, Null, DL, TLI);

test/Transforms/InstCombine/instCombineOptimizeAddrspaceCast.ll
1

The test name does not need to contain "instCombine". It can simply be called OptimizeAddrspaceCast.ll

Modified according to Reviewer's suggestions.

Modified the space formatting according to llvm convention.

Hi Reviewers,
Any comment on the updated version ?

I have made all the changes suggested by David, Meador and Mandeep. Can I commit this change now ?

Your test doesn't parse on trunk LLVM:
opt: <stdin>:14:32: error: expected comma after load's type

%2 = load i32 addrspace(4)** %ptr, align 8
majnemer requested changes to this revision.Dec 14 2015, 11:06 AM
majnemer added a reviewer: majnemer.
This revision now requires changes to proceed.Dec 14 2015, 11:06 AM
optimusPrime edited edge metadata.

Hi David,
Corrected the test and tested against trunk. Can you take a look ?

mkuper added a subscriber: mkuper.Dec 17 2015, 5:08 AM

Can you please include additional context in the diff, as requested above?
(See http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface for directions.)

lib/Analysis/ConstantFolding.cpp
1144

I'm not sure I understand why this is correct.

My understanding of the semantics of addrspacecast is that it may change the integer value of the pointer ("It can be a no-op cast or a complex value modification, depending on the target and the address space pair"). Since icmp on pointers compares integer values ("If the operands are pointer typed, the pointer values are compared as if they were integers."), this transformation may change the result of the comparison.

Am I missing something?

test/Transforms/InstCombine/OptimizeAddrspaceCast.ll
26 ↗(On Diff #43056)

Any chance to reduce the test case? It looks very large, compared to what it's actually checking.

optimusPrime edited edge metadata.

Added additional context.

mkuper removed a subscriber: mkuper.Jun 6 2016, 5:34 PM