This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Make use of analysis in the MustBeExecutedExplorer
ClosedPublic

Authored by okura on Mar 18 2020, 11:50 AM.

Details

Summary

This commit was made to settle this issue on GitHub.
I added analysis getters for LoopInfo, DominatorTree, and PostDominatorTree. And I added a test to show an improvement of the deduction of dereferenceable attribute.

Diff Detail

Event Timeline

okura created this revision.Mar 18 2020, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 11:50 AM
sstefan1 added inline comments.Mar 18 2020, 12:12 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
564

Maybe I misunderstood something, but isn't this supposed to return existing analysis results instead of creating new ones?

sstefan1 added inline comments.Mar 18 2020, 2:44 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
564

my bad, sorry

I think Stefan is correct. AnalysisGetter provides such functionality.

Thank you both of you. I will change to get an analysis through AnalysisGetter.

okura updated this revision to Diff 251291.Mar 18 2020, 11:55 PM

I changed to get analyses through AnalysisGetter.

okura updated this revision to Diff 251295.Mar 19 2020, 12:07 AM

fix directive in a test

uenoku accepted this revision.Mar 19 2020, 12:31 AM

LGTM

llvm/include/llvm/Transforms/IPO/Attributor.h
566

nit: //LIGetter -> /* LIGetter*/
https://llvm.org/docs/CodingStandards.html#comment-formatting
Other arguments are as well.

This revision is now accepted and ready to land.Mar 19 2020, 12:31 AM
okura updated this revision to Diff 251326.Mar 19 2020, 3:35 AM

fixed comment style

okura added inline comments.Mar 19 2020, 3:41 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
566

I'm sorry for not following the standards. I fixed them.

We have no other test changes? Unfortunate.

llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

TBH, I'm confused why this works. this test is only run in the old pass manager which doesn't support querying analysis passes (from the Attributor). Where am I wrong?

baziotis added inline comments.Mar 19 2020, 3:46 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

That makes sense. I tested locally and it doesn't pass, maybe I miss something too though.

uenoku requested changes to this revision.Mar 19 2020, 4:03 PM
uenoku added inline comments.
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

Sorry, I have missed this test uses OPM. It doesn't pass locally.

This revision now requires changes to proceed.Mar 19 2020, 4:03 PM
sstefan1 added inline comments.Mar 19 2020, 4:03 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

Could be that he didn't re-run the tests, since it should have worked with previous diff that didn't use AnalysisGetter.

jdoerfert added inline comments.Mar 19 2020, 5:35 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

FWIW, the solution here is to verify this works in the new pass manager ;), hence add a run line and different check lines for the new PM

baziotis added inline comments.Mar 19 2020, 6:06 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

Just to note: To do that, replace -attributor with -passes=attributor. I think it's difficult to find it, I didn't know until @uenoku told me.

okura marked an inline comment as done.Mar 19 2020, 11:59 PM
okura added inline comments.
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

As @sstefan1 mentioned, I forgot to re-run tests, and it doesn't pass. I am sorry for my carelessness. I don't know much about the pass manager, so I'm not sure whether I have to add a new RUN line or replace it.

sstefan1 added inline comments.Mar 20 2020, 3:19 AM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

yes, add a new RUN line. You can take a look at align.ll for example

okura updated this revision to Diff 251607.Mar 20 2020, 4:28 AM

add a new RUN line

okura marked an inline comment as done.Mar 20 2020, 4:31 AM
okura added inline comments.
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

I added it. I don't know what kind of name is appropriate for the label, so I use the default prefix.

jdoerfert accepted this revision.Mar 20 2020, 8:13 AM

One Nit, LGTM (assuming it passes all tests).

llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

Make the prefix ATTRIBUTOR_CGSCC_NPM

uenoku accepted this revision.Mar 20 2020, 11:31 AM
This revision is now accepted and ready to land.Mar 20 2020, 11:31 AM
okura updated this revision to Diff 251811.EditedMar 20 2020, 5:43 PM

change prefix name

okura marked an inline comment as done.Mar 20 2020, 8:01 PM
okura added inline comments.
llvm/test/Transforms/Attributor/dereferenceable-2.ll
400

I changed the prefix.

I want to upstream but to attribute this to you I need "Firstname Lastname <email>" from you

okura added a comment.Apr 1 2020, 9:07 PM

I want to upstream but to attribute this to you I need "Firstname Lastname <email>" from you

Shinji Okumura okuraofvegetable@gmail.com

This revision was automatically updated to reflect the committed changes.