This is an archive of the discontinued LLVM Phabricator instance.

folding compares if pointers do not escape
ClosedPublic

Authored by anna on Apr 19 2016, 11:24 AM.

Details

Summary

This is the first change in a series of changes to fold away comparisons when the pointer does not escape.

If we know that the pointer allocated within a function does not escape, we can fold away comparisons that are done with global pointers

Diff Detail

Repository
rL LLVM

Event Timeline

anna updated this revision to Diff 54230.Apr 19 2016, 11:24 AM
anna retitled this revision from to folding compares if pointers do not escape.
anna updated this object.
anna set the repository for this revision to rL LLVM.
anna updated this object.
anna added subscribers: llvm-commits, majnemer.
sanjoy edited edge metadata.Apr 19 2016, 12:44 PM

Minor nits inline.

lib/Transforms/InstCombine/InstructionCombining.cpp
1865

I'd make this more specific: isNeverEqualToUnescapedAlloc (unless you want to extend this in ways that the compare would fold to false).

1871

Nit: Capitalization in comment.

1873

This could be return isa<GlobalVariable>(LI->getPointerOperand());

1907

I'm not sure you need a dyn_cast<> here. Have you tried just having CmpOp0 be a Value *?

1908

LHS and OtherOp may be slightly better names than CmpOp0 and CmpOp1. Personally, I'd even elide LHS altogether, and use a ternary to compute the index of the "other" operand.

1913

Nit: capitalize comment

mcrosier added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
1871

Please include a period as well.

1913

Please include a period as well.

mgrang added a subscriber: mgrang.Apr 19 2016, 1:22 PM
mgrang added inline comments.
test/Transforms/InstCombine/compare-unescaped.ll
27

Extra newline.

anna marked 5 inline comments as done.Apr 19 2016, 1:51 PM
anna added inline comments.
lib/Transforms/InstCombine/InstructionCombining.cpp
1907

cast not needed anymore, direct comparison in ternary condition with PI

anna updated this revision to Diff 54394.Apr 20 2016, 11:18 AM
anna removed rL LLVM as the repository for this revision.
anna marked 3 inline comments as done.

revised diff with minor changes

sanjoy accepted this revision.Apr 20 2016, 12:30 PM
sanjoy edited edge metadata.

lgtm with one minor nit

lib/Transforms/InstCombine/InstructionCombining.cpp
1865

Nit: I'd rename CmpOp to just I -- the fact that it is a icmp operand is not pertinent here.

This revision is now accepted and ready to land.Apr 20 2016, 12:30 PM
anna updated this revision to Diff 54403.Apr 20 2016, 12:37 PM
anna edited edge metadata.
anna marked an inline comment as done.
majnemer added inline comments.Apr 20 2016, 1:55 PM
lib/Transforms/InstCombine/InstructionCombining.cpp
1906

Could you handle this case in isNeverEqualToUnescapedAlloc by changing it's prototype to be a Value * instead of an Instruction *?

1907

Please clang-format this line.

anna updated this revision to Diff 54434.Apr 20 2016, 3:16 PM
anna marked 2 inline comments as done.
majnemer accepted this revision.Apr 20 2016, 3:23 PM
majnemer edited edge metadata.

LGTM with nits.

lib/Transforms/InstCombine/InstructionCombining.cpp
1868

Please use auto * on the left hand side if the code makes it easy to figure out what the type is from the right hand side.

test/Transforms/InstCombine/compared-unescaped.ll
1 ↗(On Diff #54434)

I'd recommend rewriting the pipeline to redirect %s into opt like so:

opt -instcombine -S < %s | FileCheck %s

Paths make their way into opt's output sometimes and you could get confusing results if folks stick their LLVM checkout in weird places.

3 ↗(On Diff #54434)

space before the equals token.

17 ↗(On Diff #54434)

space before the curly brace.

reames added inline comments.Apr 20 2016, 5:16 PM
test/Transforms/InstCombine/compared-unescaped.ll
35 ↗(On Diff #54434)

This test case is a great example of what I mentioned to you on the phone the other day. Regardless of whether we can elide the call to malloc, we should be able to fold this comparison to false.

To be clear, I am not suggesting you change the current change (LGTM as well). This would be useful future work though.

majnemer added inline comments.Apr 20 2016, 5:37 PM
test/Transforms/InstCombine/compared-unescaped.ll
35 ↗(On Diff #54434)

Perhaps a FIXME should get added?

anna updated this revision to Diff 54501.Apr 21 2016, 7:26 AM
anna edited edge metadata.
anna marked 5 inline comments as done.
anna added inline comments.
test/Transforms/InstCombine/compared-unescaped.ll
35 ↗(On Diff #54434)

Agreed. We already have the check for nocapture operands in PointerMayBeCaptured. This specific case may be ok to add in the icmp case in PointerMayBeCaptured.

Also, I think the call to malloc cannot be elided in this case (since it may actually be leveraging the functionality of malloc + the function f deopt call). The malloc call will be elided if the IR is such that the call to function f is dce'd based on %cmp .

anna added a comment.Apr 21 2016, 7:32 AM

We cannot see the latest comments about the test file in the current diff, because I changed the file name to 'compare-unescaped.ll' instead of 'compared-unescaped.ll' :(
I have marked all comments as done in diff 4.

This revision was automatically updated to reflect the committed changes.
Allen added a subscriber: Allen.Mar 7 2022, 8:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 7 2022, 8:53 AM