Page MenuHomePhabricator

[Attributor] Make use of analysis in the MustBeExecutedExplorer
AcceptedPublic

Authored by okura on Wed, Mar 18, 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.Wed, Mar 18, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Mar 18, 11:50 AM
sstefan1 added inline comments.Wed, Mar 18, 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.Wed, Mar 18, 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.Wed, Mar 18, 11:55 PM

I changed to get analyses through AnalysisGetter.

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

fix directive in a test

uenoku accepted this revision.Thu, Mar 19, 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.Thu, Mar 19, 12:31 AM
okura updated this revision to Diff 251326.Thu, Mar 19, 3:35 AM

fixed comment style

okura added inline comments.Thu, Mar 19, 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
401

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.Thu, Mar 19, 3:46 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
401

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

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

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

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

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.Thu, Mar 19, 5:35 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
401

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.Thu, Mar 19, 6:06 PM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
401

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.Thu, Mar 19, 11:59 PM
okura added inline comments.
llvm/test/Transforms/Attributor/dereferenceable-2.ll
401

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.Fri, Mar 20, 3:19 AM
llvm/test/Transforms/Attributor/dereferenceable-2.ll
401

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

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

add a new RUN line

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

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.Fri, Mar 20, 8:13 AM

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

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

Make the prefix ATTRIBUTOR_CGSCC_NPM

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

change prefix name

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

I changed the prefix.