This is an archive of the discontinued LLVM Phabricator instance.

Replace ScalarEvolution based domain generation
ClosedPublic

Authored by jdoerfert on Aug 31 2015, 12:41 PM.

Details

Summary

This patch replaces the second legacy part of the domain generation, namely
the ScalarEvolution part that was used to obtain loop bounds. Alternatively,
we iterate over the loops in the region and propagate the back edge condition
to the header blocks. Afterwards we propagate the new information once
through the whole region.

                                                                                                                                             
- This patch already identified a couple of broken unit tests we had for
  years.
- We allow more loops already and the step to multiple exit and multiple back 
  edges is minimal.
- It allows to model the overflow checks properly as we actually visit
  every block in the SCoP and know where which condition is evaluated.

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 33613.Aug 31 2015, 12:41 PM
jdoerfert retitled this revision from to [WIP] Replace ScalarEvolution based domain generation.
jdoerfert added reviewers: grosser, mssimpso, Meinersbur.
jdoerfert updated this object.
jdoerfert added a subscriber: Restricted Project.

Some comments to understand the test changes.

test/Isl/CodeGen/OpenMP/loop-body-references-outer-values-3.ll
12 ↗(On Diff #33613)

There was an if (cols >= 2) around this loop and a Stmt_for_body(0) in the else case.
The conditional is gone in the new version.

42 ↗(On Diff #33613)

There was an if (cols >= 2) around this loop and a Stmt_for_body(0) in the else case.
The conditional is gone in the new version.

test/Isl/CodeGen/OpenMP/reference-preceeding-loop.ll
17 ↗(On Diff #33613)

This test was funny, p_0 is actually -p_2, hence the loop had one iteration only, namely Stmt_while_body(0)

test/Isl/CodeGen/two-scops-in-row.ll
11 ↗(On Diff #33613)

Similar to the case above, Stmt_for_1(0) is always executed here but it is explicit in the new version

test/ScopInfo/isl_trip_count_01.ll
3 ↗(On Diff #33613)

Here the original test case is actually broken. If you look at the code but ignore the entry node (it is not in the SCoP!) than you see that for N=0 and M=1 we would execute while_body but the domain says otherwise.

test/ScopInfo/loop_affine_bound_0.ll
67 ↗(On Diff #33613)

This is redundant as it can be extracted from the rest.

test/ScopInfo/loop_affine_bound_1.ll
19 ↗(On Diff #33613)

This was just to make clear that the original code did not use the entry condition but just generates the wrong domain (similar to the isl_trip_count_01.ll above).

66 ↗(On Diff #33613)

Again a broken test for M=-1 and N=0.

test/ScopInfo/loop_affine_bound_2.ll
77 ↗(On Diff #33613)

Again redundant.

grosser accepted this revision.Sep 1 2015, 11:54 AM
grosser edited edge metadata.

Hi Johannes,

this looks good for me so far.

include/polly/ScopInfo.h
559 ↗(On Diff #33613)

It seems you could drop some parts of TempScopInfo now, as they now become unused/dead-code.

912 ↗(On Diff #33613)

argue? This verb does not seem to make sense in this context.

lib/Analysis/ScopInfo.cpp
817 ↗(On Diff #33613)

bset, maybe uppercase?

828 ↗(On Diff #33613)

Please comment the contents of the return value.

831 ↗(On Diff #33613)

Uppercase iterators?

841 ↗(On Diff #33613)

Just removing dimensions is always fishy as this is just an operation of the internal set representation, so bad things can happen. I am not yet sure why this
is needed at all, but if it indeed is maybe you wanna use projection.

845 ↗(On Diff #33613)

Unnecessary line break

857 ↗(On Diff #33613)

Maybe extract out a function that collects the bounded parts of an isl_set.

1705 ↗(On Diff #33613)

Please clarify that this propagation is not done later in this function itself, but is expected to be done afterwards (by another funciton) after this function has finished.

Also, the name of this function is misleading. Maybe call it addLoopBoundsToHeaderDomains

1729 ↗(On Diff #33613)

Why an empty line?

1753 ↗(On Diff #33613)

Please use project instead of remove_dims, as this does not depend on the representation of the isl_set.

This revision is now accepted and ready to land.Sep 1 2015, 11:54 AM
jdoerfert updated this revision to Diff 34326.Sep 9 2015, 6:35 AM
jdoerfert edited edge metadata.

Both lnt + unit tests work, however pointer & modulo expressions are disabled

Hi Johannes,

some smaller comments only:

lib/Analysis/ScopDetection.cpp
351 ↗(On Diff #34326)

Can you please commit this separately.

lib/Analysis/ScopInfo.cpp
1 ↗(On Diff #34326)

You drop the header comment here accidentally.

lib/Support/SCEVValidator.cpp
357 ↗(On Diff #34326)

Interesting. I do not care so much about loop bounds/conditions, but it would be sad to loose support even in array index expressions. To my understanding, srem instructions in index expressions should not cause troubles in our domain generation algorithm, no?

jdoerfert retitled this revision from [WIP] Replace ScalarEvolution based domain generation to Replace ScalarEvolution based domain generation.Sep 9 2015, 7:22 AM
jdoerfert updated this object.
jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.
lib/Support/SCEVValidator.cpp
357 ↗(On Diff #34326)

Correct. Any ideas on how to allow those but forbid srem in domain related expressions?

jdoerfert updated this revision to Diff 34332.Sep 9 2015, 7:23 AM
jdoerfert updated this object.

Modified according to comments.

Meinersbur added inline comments.Sep 9 2015, 8:05 AM
lib/Analysis/ScopInfo.cpp
805 ↗(On Diff #34332)

artificial

1451 ↗(On Diff #34332)

Function extraction into separate commit?

1546 ↗(On Diff #34332)

Is dropping DEBUG()s this part of the patch?

1590 ↗(On Diff #34332)

Empty comment line/missing annotations

1690 ↗(On Diff #34332)

hence -> e.g.

1775 ↗(On Diff #34332)

isl_set *&
LLVM coding standards: "Use auto if and only if it makes the code more readable or easier to maintain"
In any case, if you keep auto, you can also let it derive that it's a pointer.

lib/Support/SCEVValidator.cpp
356 ↗(On Diff #34332)

Adding a boolean parameter to SCEVValidator's constructor to discriminate the two cases?

I don't get why it's not compatible for domains.

357 ↗(On Diff #34332)

Don't commit out-commented source
Also, use

#if 0
#endif

for such cases

test/Isl/CodeGen/OpenMP/loop-body-references-outer-values-3.ll
42 ↗(On Diff #34332)

nice

Meinersbur added inline comments.Sep 9 2015, 8:59 AM
lib/Support/SCEVValidator.cpp
356 ↗(On Diff #34332)

We could generate a condition inside the body

for (int i = 0; i < n; i+=1) {
   if (i % 3 <= 1)
     continue;
  // stmt(i)
}

for a domain s.a. { [i] : 0 <= i < n, i - floord(i,3) * 3 <= 1 }

Alternatively, map from sequential loop counter to domain index:

for (int i_seq = 0; i_seq < (n*2/3); i_seq+=1) {
   int i = i_seq / 2 * 3 + i_seq % 2;
  // stmt(i)
}

(probably some edge-case errors in this example)

jdoerfert updated this revision to Diff 34374.Sep 9 2015, 3:41 PM
jdoerfert marked 7 inline comments as done.

Addressed comments by Michael

This revision was automatically updated to reflect the committed changes.