Page MenuHomePhabricator

[GVN] Add GVNOption to control load-pre more fine-grained.
ClosedPublic

Authored by hgreving on Jan 31 2020, 1:30 PM.

Details

Summary

Adds the global (cl::opt) GVNOption enable-load-in-loop-pre in order
to control whether the optimization will be performed if the load
is part of a loop.

Adds the lit test Transforms/GVN/PRE/pre-load-in-loop.ll.

Diff Detail

Event Timeline

hgreving created this revision.Jan 31 2020, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2020, 1:30 PM

Hello, this switch intends to help our backend to software pipeline single basic block loops. The suppressed GVN opt is creating control flow in loops which we like to avoid. Thanks in advance for looking at the patch.

asbirlea accepted this revision.Jan 31 2020, 2:16 PM

lgtm.

This revision is now accepted and ready to land.Jan 31 2020, 2:16 PM
fhahn added inline comments.Jan 31 2020, 2:19 PM
llvm/lib/Transforms/Scalar/GVN.cpp
1406

nit: drop unnecessary this-> ?

hgreving marked an inline comment as done.Jan 31 2020, 2:34 PM
hgreving added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
1406

This is testing the LoopInfo, not the LoadInst. Having said that, I considered changing the this->LI name into sth else since LI shadows LI in GVN::runImpl as well, but thought this is an unrelated change. Happy to submit this as well.

fhahn added inline comments.Jan 31 2020, 2:41 PM
llvm/lib/Transforms/Scalar/GVN.cpp
1406

Ah right, I didn't look carefully enough. It seems a very unfortunate name clash, but I guess this->LI is fine.

asbirlea marked an inline comment as done.Jan 31 2020, 3:30 PM
asbirlea added inline comments.
llvm/lib/Transforms/Scalar/GVN.cpp
1406

I thought to suggest renaming of either the LoopInfo or LoadInst instances in a separate cleanup patch, but thought that's more than you intended to do.

Happy to look over the patch, if you do send it out. Sadly, LoopInfo has only a couple uses here, but it's traditionally named LI (I found a single declaration called LF), and LoadInst has a very large number of uses here, and can be named L, I, LI, LoadI etc.
So, *shrug* use your best judgment if either of renamings is worth doing.

May I ask you to add a support for new pass manager to specify new introduced option in command line?

For details, see PassRgistry.def and PassBuilder.cpp:parseGVNOptions.

fedor.sergeev added inline comments.
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
2

It makes sense to add both positive - checking that load-pre is performed when enabled, and negative - checking that load-pre does not happen when disabled
(say, to catch a case when load-pre is not performed due to some bug or change in other defaults).

fedor.sergeev added a comment.EditedFeb 2 2020, 10:02 PM

Hello, this switch intends to help our backend to software pipeline single basic block loops. The suppressed GVN opt is creating control flow in loops which we like to avoid. Thanks in advance for looking at the patch.

Curiously, just recently we had a similar case in our downstream pipeline, though we had a GVN splitting loop backedge and leading to a loss of a loop form (by making a non-exiting latch).
Solved by disabling load-pre in a separate responsible GVN instance, though this control looks more appropriate.

I wonder whether this change needs to be extended into "not doing questionable control flow modifications when loop-pre'ing in loops" and enabled unconditionally?
It will require properly defining this "questionable" part.

hgreving marked 2 inline comments as done.Feb 3 2020, 7:39 AM
hgreving added inline comments.
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
2

The positive case is checked in load-pre.ll. Does this suffice? WDYT?

fedor.sergeev added inline comments.Feb 3 2020, 2:24 PM
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
2

Shouldnt then -enable-load-in-loop-pre=true be added explicitly to load-pre.ll, to avoid its behavior being dependant on default setting?

38

Correct the comment to match behavior.

hgreving marked 2 inline comments as done.Feb 3 2020, 2:51 PM
hgreving added inline comments.
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
2

I think it _should_ depend on the default setting, as this should be considered a switch that explicitly diverts from the default IMO. I _can_ add this to loop-pre.ll, do you really want this? Either way ok with me.

hgreving marked 2 inline comments as done.Feb 3 2020, 2:52 PM
hgreving updated this revision to Diff 242214.Feb 3 2020, 3:16 PM

llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll:
s/Should only be one load in the loop./Both loads should remain in the loop./

hgreving marked 2 inline comments as done and an inline comment as not done.Feb 3 2020, 3:17 PM
hgreving added inline comments.
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
38

Done, thanks!

This revision was automatically updated to reflect the committed changes.
fedor.sergeev added inline comments.Feb 3 2020, 11:09 PM
llvm/test/Transforms/GVN/PRE/pre-load-in-loop.ll
2

The point is that defaults do change.
And I do see some variation of load-pre-in-loop restriction being quite useful by default.
(see my earlier comment on possibly enabling this unconditionally).

So it is quite normal to have a test that explicitly tests both true and false, just to avoid a hassle of updating tests upon a toggle of default value.