[PATCH] D9401: llvm.noalias - The AA implementaton

[Ehsan Amiri via Phabricator via llvm-commits]

amehsan added inline comments.


================
Comment at: lib/Analysis/ScopedNoAliasAA.cpp:382
+  DEBUG(dbgs() << "SNA: A: " << *APtr << "\n");
+  DEBUG(dbgs() << "SNA: "; if (CSA) dbgs() << "CSB: " << *CSB.getInstruction();
+                           else dbgs() << "B: " << *BPtr; dbgs() << "\n");
----------------
I think it should be if (CSB)


================
Comment at: lib/Analysis/ScopedNoAliasAA.cpp:436-451
+  SmallVector<MetadataAsValue *, 8> CompatibleSetMVs;
+  for (auto &C : CompatibleSet)
+    CompatibleSetMVs.push_back(cast<MetadataAsValue>(C->getOperand(1)));
+  for (auto &MV : CompatibleSetMVs)
+    for (Use &U : MV->uses())
+      if (auto *UI = dyn_cast<Instruction>(U.getUser()))
+        if (CompatibleSetMembers.insert(UI).second) {
----------------
IIUC, with the logic that we have here, removing a llvm.noalias from the IR could be a functional problem. The example below can expose the issue (AFAICT, the example has no UB...but please double check.


```
void foo(int *a, int *b, int *c, int *d, int n)  {

  int *restrict s;

  for (int i = 10; i < 800; ++i) {
    s = c + b[i];
    *a += *s;
  }


  int *r = s;
  s = d + 8;

  *s = *r + *s;

}

```

I compile this with the following options: -O2 -S   -emit-llvm -fno-unroll-loops. Then I manually remove the llvm.noalias intrinsic inside the loop body and make necessary changes. This is the interesting parts of the IR


```
entry:
  br label %for.body

for.cond.cleanup:                                 ; preds = %for.body
  %add.ptr1 = getelementptr inbounds i32, i32* %d, i64 8
  %0 = tail call i32* @llvm.noalias.p0i32(i32* %add.ptr1, metadata !1)

for.body:                                         ; preds = %for.body, %entry
  %add.ptr = getelementptr inbounds i32, i32* %c, i64 %idx.ext
}

```

Then I feed the manually modified IR to opt, with options -O2 -debug and I see this debug msg:


```
SNA: A:   %0 = tail call i32* @llvm.noalias.p0i32(i32* %add.ptr1, metadata !5)
SNA: B:   %add.ptr = getelementptr inbounds i32, i32* %c, i64 %idx.ext
SNA: Found a compatible set!
          %0 = tail call i32* @llvm.noalias.p0i32(i32* %add.ptr1, metadata !5)

SNA: B/CSB noalias:

SNA: B does not derive from the compatible set!
SNA: DT is available
 SNA: noalias!

```
So we seem to prove %0 and %add.ptr are not aliased, but they might be aliased legally, IIUC. 

For some more interesting examples, the problem is not exposed because currently at the very end of BasicAAResult::aliasGEP we conservatively return PartialAlias.





https://reviews.llvm.org/D9401