This is an archive of the discontinued LLVM Phabricator instance.

Use alias analysis to check for real interference in cascade comparison
AbandonedPublic

Authored by christylee on Aug 31 2018, 10:51 AM.

Details

Summary

Passing in AliasAnalysis to MergeICmps, we can check for real interference using alias analysis when some other work is done in a comparison block.

Diff Detail

Event Timeline

christylee created this revision.Aug 31 2018, 10:51 AM

This looks good to me. I will let @courbet to have the final say here.

Thanks for the patch Christy !

lib/Transforms/Scalar/MergeICmps.cpp
200

What about instructions that do not write to memory but may throw ?

test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
11

It's not clear what "this call" refers to.

For instructions that may have side effects, check that the instruction is a simple load or simple store before checking for alias. For things like function calls, we would have to check that they don't throw and have no read/write to memory. The function calls are potentially nested as well, it's probably better to just bail on those cases.

christylee marked 2 inline comments as done.Sep 10 2018, 11:58 AM

Removed clang-format changes unrelated to this patch.

lebedev.ri added inline comments.
test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

Would be good to commit the test with current check-lines, so the diff is visible in the review.

christylee added inline comments.Sep 10 2018, 1:36 PM
test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

Hi @lebedev.ri ! Sorry I'm new to LLVM, what do you mean by "check-lines"?

lebedev.ri added inline comments.Sep 10 2018, 1:42 PM
test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

In other words, i was suggesting to commit this test file (with the correct output from
utils/update_test_checks.py), so this differential would then show
how the code change affects the test.

check-lines - the lines with prefixes specified in --check-prefix= of FileCheck, in this case, ; X86

christylee added inline comments.
test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

Is this what you mean, @lebedev.ri ?

courbet added inline comments.Sep 11 2018, 7:35 AM
lib/Transforms/Scalar/MergeICmps.cpp
202

add a comment: "// Disallow stores that might alias the BCE operands"

test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

I think what Roman means is: create another diff with just this file in it. Then the current diff will show what your change does.

Addressed @courbet comment

Addressed @lebedev.ri 's comment and added dependent child diff to show original check-lines.

Addressed @lebedev.ri 's comment and added dependent child diff to show original check-lines.

This is slightly backwards.
That one needs to be the parent differential, and this differential needs to be based on that one,
and show how the check lines are changed by this diff.

Fixed diff dependencies.

Phabricator is dumb, it only shows the patch you have uploaded, it does not diff anything.
So you need to manually generate this diff to be \as compared to D52044'.
In other words, this differential should not be adding the alias-merge-blocks.ll,
it should only show how the check-lines change in it.

lebedev.ri added inline comments.Sep 13 2018, 11:57 AM
test/Transforms/MergeICmps/X86/alias-merge-blocks.ll
2

This current revision is what i meant :) (other than -U99999)

courbet accepted this revision.Sep 17 2018, 12:15 AM
This revision is now accepted and ready to land.Sep 17 2018, 12:15 AM

Going back to previous revision to include the test in this revision.

@courbet , @lebedev.ri , I'm trying to commit the patch by first running "arc patch D51550", but it keeps saying unable to apply patch because llvm/trunk/test/Transforms/MergeICmps/X86/alias-merge-blocks.ll does not exists in index. How should I go about this?

(master)$ llvm-arc patch D51550
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D51550.
 INFO  Base commit is not in local repository; trying to fetch.
Checking patch llvm/trunk/test/Transforms/MergeICmps/X86/alias-merge-blocks.ll...
error: llvm/trunk/test/Transforms/MergeICmps/X86/alias-merge-blocks.ll: does not exist in index

 Patch Failed!
Usage Exception: Unable to apply patch!
christylee abandoned this revision.Sep 24 2018, 1:51 PM

The changes were landed in D52433