This is an archive of the discontinued LLVM Phabricator instance.

DA: remove uses of GEP, only ask SCEV
ClosedPublic

Authored by sebpop on Jul 14 2017, 11:59 AM.

Details

Summary

It's been quite some time the Dependence Analysis (DA) is broken,
as it uses the GEP representation to "identify" multi-dimensional arrays.
It even wrongly detects multi-dimensional arrays in single nested loops:

from test/Analysis/DependenceAnalysis/Coupled.ll, example @couple6
;; for (long int i = 0; i < 50; i++) {
;; A[i][3*i - 6] = i;
;; *B++ = A[i][i];

DA used to detect two subscripts, which makes no sense in the LLVM IR
or in C/C++ semantics, as there are no guarantees as in Fortran of
subscripts not overlapping into a next array dimension:

maximum nesting levels = 1
SrcPtrSCEV = %A
DstPtrSCEV = %A
using GEPs
subscript 0
    src = {0,+,1}<nuw><nsw><%for.body>
    dst = {0,+,1}<nuw><nsw><%for.body>
    class = 1
    loops = {1}
subscript 1
    src = {-6,+,3}<nsw><%for.body>
    dst = {0,+,1}<nuw><nsw><%for.body>
    class = 1
    loops = {1}
Separable = {}
Coupled = {1}

With the current patch, DA will correctly work on only one dimension:

maximum nesting levels = 1
SrcSCEV = {(-2424 + %A)<nsw>,+,1212}<%for.body>
DstSCEV = {%A,+,404}<%for.body>
subscript 0
    src = {(-2424 + %A)<nsw>,+,1212}<%for.body>
    dst = {%A,+,404}<%for.body>
    class = 1
    loops = {1}
Separable = {0}
Coupled = {}

This change removes all uses of GEP from DA, and we now only rely
on the SCEV representation.

The patch does not turn on -da-delinearize by default, though without
delinearization, and so the DA analysis will be more conservative in the
case of multi-dimensional memory accesses in nested loops.

I disabled some interchange tests, as the DA is not able to disambiguate
the dependence anymore. To make DA stronger, we may need to
compute a bound on the number of iterations based on the access functions
and array dimensions.

Patch written by Sebastian Pop and Aditya Kumar.

Diff Detail

Repository
rL LLVM

Event Timeline

sebpop updated this revision to Diff 106687.Jul 14 2017, 11:59 AM
sebpop created this revision.
grosser accepted this revision.Jul 17 2017, 8:18 PM

LGTM. Thank you Sebastian!

This revision is now accepted and ready to land.Jul 17 2017, 8:18 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Mar 13 2018, 9:46 AM

As it is, this change unfortunately reduced the test coverage for
LoopInterchange quite a bit.

Requiring assertions for the tests is not too bad, although I think it would be better to use optimization remarks, which should be available in release mode too.

whenever it was possible to use -pass-remarks=loop-interchange I used that.
The loop interchange pass needs to output more diagnostic about why it failed to interchange when using -pass-remarks.
Then the tests can be moved back to not require asserts.

But we definitely need at least some tests that check the actual IR, to make sure the actual transformation is performed (or not performed) and correct.

What about adding new correctness tests for loop interchange to the test-suite?

Requiring assertions for the tests is not too bad, although I think it would be better to use optimization remarks, which should be available in release mode too.

whenever it was possible to use -pass-remarks=loop-interchange I used that.
The loop interchange pass needs to output more diagnostic about why it failed to interchange when using -pass-remarks.
Then the tests can be moved back to not require asserts.

Yes, unfortunately there is lots of work to do...

But we definitely need at least some tests that check the actual IR, to make sure the actual transformation is performed (or not performed) and correct.

What about adding new correctness tests for loop interchange to the test-suite?

That's an excellent idea. I have a patch lying around somewhere and this should be enough motivation to submit it for review ;-)

I think we still need at least some tests checking interchanging is done correctly on the IR level. I'll look into cleaning the tests up in general.

fhahn added a comment.Apr 3 2018, 10:48 AM

With this change, we cannot proof dependences in cases the loop bounds are not known, e.g.

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@A = common global [100 x [100 x i64]] zeroinitializer

;;  for(int i=0;i<100;i++)
;;    for(int j=0;j<100;j++)
;;      A[j][i] = A[j][i]+k;

define void @interchange_01(i64 %k, i64 %N) {
entry:
  br label %for1.header

for1.header:
  %j23 = phi i64 [ 0, %entry ], [ %j.next24, %for1.inc10 ]
  br label %for2

for2:
  %j = phi i64 [ %j.next, %for2 ], [ 0, %for1.header ]
  %arrayidx5 = getelementptr inbounds [100 x [100 x i64]], [100 x [100 x i64]]* @A, i64 0, i64 %j, i64 %j23
  %arrayidx6 = getelementptr inbounds [100 x [100 x i64]], [100 x [100 x i64]]* @A, i64 0, i64 %j, i64 %j23
  %lv = load i64, i64* %arrayidx5
  %add = add nsw i64 %lv, %k
  store i64 %add, i64* %arrayidx6
  %j.next = add nuw nsw i64 %j, 1
  %exitcond = icmp eq i64 %j, %N
  br i1 %exitcond, label %for1.inc10, label %for2

for1.inc10:
  %j.next24 = add nuw nsw i64 %j23, 1
  %exitcond26 = icmp eq i64 %j23, %N
  br i1 %exitcond26, label %for.end12, label %for1.header

for.end12:
  ret void
}

I am probably missing something, but couldn't we just calculate DstSCEV - SrcSCEV using scalar evolution. If it is a zero, wouldn't that show that there is an EQ dependence in that case? And wouldn't the sign of a non zero constant indicate the direction?

llvm/trunk/test/Transforms/LoopInterchange/interchange-insts-between-indvar.ll