This is an archive of the discontinued LLVM Phabricator instance.

[GVN] introduce GVNOptions to control GVN pass behavior
ClosedPublic

Authored by fedor.sergeev on Jan 14 2020, 1:12 PM.

Details

Summary

There are a few global (cl::opt) controls that enable optional
behavior in GVN. Introduce GVNOptions that provide corresponding
per-pass instance controls.

That will allow to use GVN multiple times in pipeline each time
with different settings.

Diff Detail

Event Timeline

fedor.sergeev created this revision.Jan 14 2020, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 1:12 PM

Thanks, this looks useful!

llvm/include/llvm/Transforms/Scalar/GVN.h
90

Update comment?

llvm/lib/Transforms/Scalar/GVN.cpp
637

Conditionally getResult<MemDepAnalysis> in a follow up change?

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

updated comments

fedor.sergeev marked 2 inline comments as done.Jan 14 2020, 11:02 PM
fedor.sergeev added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
637

Yep, will do as a separate one-liner after this patch.

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

Don't you want to add a code to be able to set GVN options from command line for new pass manager? Or it is a follow-up change?

That should go as a followup.
I did not plan to do that right now, but well, you got a point.
Will post a review for that.

I'm fine with follow-up.

gchatelet added a subscriber: gchatelet.

Removing myself from the reviewers (I'm not an owner of this code)

fhahn added inline comments.Jan 15 2020, 9:24 AM
llvm/lib/Transforms/Scalar/GVN.cpp
2724

This could use Impl. isMemDepEnabled as well, right? Then we could get rid of the NoMemDepAnalysis variable.

fixing NoMemDepAnalysis use in legacy pass

fhahn accepted this revision.Jan 15 2020, 2:07 PM

LGTM, thanks. Please wait a day or so with committing, in case there are additional comments.

This revision is now accepted and ready to land.Jan 15 2020, 2:07 PM
fedor.sergeev marked 3 inline comments as done.Jan 15 2020, 2:08 PM
fedor.sergeev added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
2696–2697

Also removed default value as it is not used anyway and just gives a wrong feeling that global control affects something here.
In fact it was not (and I just kept pre-existing behavior).

2724

thanks for catching this!

fedor.sergeev marked an inline comment as done.

reformatting after the last update

fedor.sergeev marked an inline comment as done.Jan 15 2020, 2:20 PM
fedor.sergeev added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
637

since this patch became rather involved on legacy part anyway,
I decided that doing this change here (and not in a followup) is fine.

fhahn added inline comments.Jan 16 2020, 2:21 AM
llvm/lib/Transforms/Scalar/GVN.cpp
637

Sounds good to me.

reintroduced default parameter for Legacy pass constructor
(it is necessary for PassSupport.h proper operations and not otherwise used)

This revision was automatically updated to reflect the committed changes.