This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Consolidate invariant loads
ClosedPublic

Authored by jdoerfert on Oct 1 2015, 4:13 AM.

Details

Summary
                                                                                                                                               
If a (assumed) invariant location is loaded multiple times we
generated a parameter for each location. However, this caused compile
time problems for several benchmarks (e.g., 445_gobmk in SPEC2006 and
BT in the NAS benchmarks). Additionally, the code we generate is
suboptimal as we preload the same location multiple times and perform
the same checks on all the parameters that refere to the same value.

With this patch we consolidate the invariant loads in three steps:
  1) During SCoP initialization required invariant loads are put in
     equivalence classes based on their pointer operand. One
     representing load is used to generate a parameter for the whole
     class, thus we never generate multiple parameters for the same
     location.
  2) During the SCoP simplification we remove invariant memory
     accesses that are in the same equivalence class. While doing so
     we build the union of all execution domains as it is only
     important that the location is at least accessed once.
  3) During code generation we only preload one element of each
     equivalence class with the unified execution domain. All others
     are mapped to that preloaded value.
     equivalence classes based on their pointer operand. One
     representing load is used to generate a parameter for the whole
     class, thus we never generate multiple parameters for the same
     location.

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 36221.Oct 1 2015, 4:13 AM
jdoerfert retitled this revision from to [Polly] Consolidate invariant loads.
jdoerfert added reviewers: grosser, Meinersbur.
jdoerfert updated this object.
jdoerfert added a subscriber: Restricted Project.
jdoerfert updated this revision to Diff 36222.Oct 1 2015, 4:15 AM

Diff against D13195 to highlight the changes

Meinersbur edited edge metadata.Oct 1 2015, 5:57 AM

Could you please describe what the code is doing not only in the commit message, but also as source code comment?

Why is ScopDetection involved at all? Shouldn't it be ScopInfo alone which decides what that Scop's parameters are?

Could you please describe what the code is doing not only in the commit message, but also as source code comment?

I think the source is well documented. If you disagree please inline a comment so I know what part you refer too.

Why is ScopDetection involved at all?

Because in ScopInfo we cannot build the equivalence classes until the SCoP is completed and to build the SCoP in resonable time we need equivalence classes.
For example in the SCEVAffinator (that is used throughout the SCoP creation) we need to normalize required invariant load parameters otherwise we would introduce different parameters for each invariant load.
To normalize these parameters we already need equivalence classes but in the expression that is translated at that point there might only be a reference to one of the invariant loads (most certainly not to all that are equivalent).
Thus, to determine the representing element for an equivalence class we need to know all elements of it before we use the SCEVAffinator for the first time.
The only way to build the equivalence classses before we use the SCEVAffinator is to do it in the ScopDetection where we actually see all required invariant loads.

Shouldn't it be ScopInfo alone which decides what that Scop's parameters are?

ScopInfo actually never "really" decides what the parameters are, it only normalizes them to a certain degree (and now even more). The parameters are collected and given to the SCoP by the SCEVAffinator and the SCEVValidator.

Could you please describe what the code is doing not only in the commit message, but also as source code comment?

I think the source is well documented. If you disagree please inline a comment so I know what part you refer too.

The commit message describes 3 phases, but there are just 1.5 notable new comments. What about the other phases?

I added some inline comments where I think you could write a bit more. (These are not questions you need to answer to me, I got them from either some other comment or the commit log).

Mmh, I mixed them with my other remarks that I found.

Why is ScopDetection involved at all?

Because in ScopInfo we cannot build the equivalence classes until the SCoP is completed and to build the SCoP in resonable time we need equivalence classes.
For example in the SCEVAffinator (that is used throughout the SCoP creation) we need to normalize required invariant load parameters otherwise we would introduce different parameters for each invariant load.
To normalize these parameters we already need equivalence classes but in the expression that is translated at that point there might only be a reference to one of the invariant loads (most certainly not to all that are equivalent).
Thus, to determine the representing element for an equivalence class we need to know all elements of it before we use the SCEVAffinator for the first time.
The only way to build the equivalence classses before we use the SCEVAffinator is to do it in the ScopDetection where we actually see all required invariant loads.

Correct me if I am wrong, SCEVAffinator is only used by the ScopInfo pass. ScopInfo also get the list of invariant using getRequiredInvariantLoads(). It can by itself create the equivalence classes by going through the map.

This might be additional work because ScopDetection in the patch does the equivalence classes already on the fly. However, it would be better for layering as ScopDetection shouldn't care about hoisting; it just determines the size of scop regions.

Shouldn't it be ScopInfo alone which decides what that Scop's parameters are?

ScopInfo actually never "really" decides what the parameters are, it only normalizes them to a certain degree (and now even more). The parameters are collected and given to the SCoP by the SCEVAffinator and the SCEVValidator.

Isn't it ScopInfo::addParams() which collect the parameters?

include/polly/ScopDetection.h
150 ↗(On Diff #36222)

What are they remembered for?

188 ↗(On Diff #36222)

What are the equivalence classes? Why are there equivalence classes?

254 ↗(On Diff #36222)

What is the condition for that?

402 ↗(On Diff #36222)

Why are they required?

include/polly/ScopInfo.h
936 ↗(On Diff #36222)

Why the rename?

1219 ↗(On Diff #36222)

If those are two actions, why not put them into different functions?
I can't find the changes for simplifySCoP().

1241 ↗(On Diff #36222)

I prefer the longer name from ScopDetection

include/polly/Support/SCEVAffinator.h
57 ↗(On Diff #36222)

What is it used for?

include/polly/Support/SCEVValidator.h
51 ↗(On Diff #36222)

@param missing

include/polly/Support/ScopHelper.h
46 ↗(On Diff #36222)

What is the order?

49 ↗(On Diff #36222)

What is the key?

152 ↗(On Diff #36222)

When is it applicable? What is the special property of the representive SCEV?

lib/Analysis/ScopDetection.cpp
302 ↗(On Diff #36222)

Describe under which condition the invariant valid load is required

326 ↗(On Diff #36222)

return onlyValidRequiredInvariantLoads(AccessILC, Context)

705 ↗(On Diff #36222)

This is 1)?

1148 ↗(On Diff #36222)

Is it dummy (could you pass NULL instead)? Or does it serve as scratch storage?

lib/Analysis/ScopInfo.cpp
1368 ↗(On Diff #36222)

Why the rename?

1424 ↗(On Diff #36222)

Ideas how to improve this?

1437 ↗(On Diff #36222)

Is this 2)? Can you describe why it is the union of the two?

1476 ↗(On Diff #36222)

Why is one parameter correct but not the other?

lib/CodeGen/IslNodeBuilder.cpp
908 ↗(On Diff #36222)

Why the rename? Doesn't "auto" know by itself that it's a const reference?

930 ↗(On Diff #36222)

This is 3) ?

lib/Support/ScopHelper.cpp
385 ↗(On Diff #36222)

Describe the conditions

jdoerfert updated this object.Oct 1 2015, 5:01 PM
jdoerfert edited edge metadata.
jdoerfert updated this revision to Diff 36323.Oct 1 2015, 5:03 PM

Updated according to Michaels idea. The SCoP will now hide most of the equivalence class magic.

Cool! Looks less impactful now!

grosser edited edge metadata.Oct 8 2015, 12:58 AM

Hi Johannes,

the patch looks conceptually good and it has a very useful commit message. I do not have any code changes, but would suggest a couple of additional comments. Some of the information that I miss as source code comments can probably just been taken from your commit message.

Some minor comments:

  • There is an incomplete sentence in part 3) of the commit message

Cool! Looks less impactful now!

@Michael: Thanks for reviewing! One point: in this message it seems you finished your review, but it is unclear if the patch is good to go, if you would prefer Johannes to still address some of your open comments or if you prefer me to have another look. It would help if you could state this explicitly.

Best,
Tobias

include/polly/ScopInfo.h
702 ↗(On Diff #36323)

ordered

939 ↗(On Diff #36323)

As Michael mentioned, the rename seems unrelated. If it is I would prefer to commit it separately before this commit to reduce the actual diff.

lib/Analysis/ScopInfo.cpp
1442 ↗(On Diff #36323)

As Michael mentioned, adding the information of part 2) of your commit message as a comment here would make the code more understandable. IAs this function is getting large, it might indeed make sense to split it into two subfunctions, each with its own comment.

1477 ↗(On Diff #36323)

You mention the term equivalence classes here and in one header, but do not explain which equivalence classes exist. It would be helpful to explicitly state at one of these locations what are the elements that are sorted into equivalence classes, how do they differ and which properties are used to sort them into equivalence classes.

1506 ↗(On Diff #36323)

Michael commented: "Why is one parameter correct but not the other?"

This does not yet seem to be addressed and is not clear to me either,

lib/CodeGen/IslNodeBuilder.cpp
914–915 ↗(On Diff #36323)

As Michael commented, this rename seems unrelated. (I like the rename, but please just commit it separately ahead of time)

@Michael: auto can derive 'const' and '*' but in some cases we still add them to make clear that something is a ptr or a const. Not sure if this information adds additional value here though.

936 ↗(On Diff #36323)

As Michael mentioned, this seems to be 3) from your commit message. Adding the information from your commit message in the source code would be useful.

This revision was automatically updated to reflect the committed changes.