[SCEV] Reject comparision of pointers to different address spaces in SCEVWrapPredicate::implies

[vikramRH]

This is an attempt to fix a crash as seen here, https://godbolt.org/z/dqYzK17bj. I'm not sure if this is too conservative/right way to handle this. would just comparing and rejecting the address sizes of different address spaces be enough ?

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Vikram Hegde (vikramRH)

Changes

This is an attempt to fix a crash as seen here, https://godbolt.org/z/dqYzK17bj. I'm not sure if this is too conservative/right way to handle this. would just comparing and rejecting the address sizes of different address spaces be enough ?

---

Full diff: https://github.com/llvm/llvm-project/pull/137935.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+6)
• (modified) llvm/test/Analysis/LoopAccessAnalysis/nusw-predicates.ll (+35-1)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 6055c3d791cb2..b39662ef90c05 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -14967,6 +14967,12 @@ bool SCEVWrapPredicate::implies(const SCEVPredicate *N,
   if (Start->getType()->isPointerTy() != OpStart->getType()->isPointerTy())
     return false;
 
+  if (Start->getType()->isPointerTy()) {
+    if (Start->getType()->getPointerAddressSpace() !=
+        OpStart->getType()->getPointerAddressSpace())
+      return false;
+  }
+
   const SCEV *Step = AR->getStepRecurrence(SE);
   const SCEV *OpStep = Op->AR->getStepRecurrence(SE);
   if (!SE.isKnownPositive(Step) || !SE.isKnownPositive(OpStep))
diff --git a/llvm/test/Analysis/LoopAccessAnalysis/nusw-predicates.ll b/llvm/test/Analysis/LoopAccessAnalysis/nusw-predicates.ll
index d4f7f82a8cff1..ab40a22a3274d 100644
--- a/llvm/test/Analysis/LoopAccessAnalysis/nusw-predicates.ll
+++ b/llvm/test/Analysis/LoopAccessAnalysis/nusw-predicates.ll
@@ -1,7 +1,7 @@
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --version 5
 ; RUN: opt -passes='print<access-info>' -disable-output %s 2>&1 | FileCheck %s
 
-target datalayout = "p:16:16"
+target datalayout = "p:16:16-p3:32:32"
 
 define void @int_and_pointer_predicate(ptr %v, i32 %N) {
 ; CHECK-LABEL: 'int_and_pointer_predicate'
@@ -124,3 +124,37 @@ loop:
 exit:
   ret void
 }
+
+define void @pointers_to_different_aspace_predicates(ptr %v, ptr addrspace(3) %w, i32 %N) {
+; CHECK-LABEL: 'pointers_to_different_aspace_predicates'
+; CHECK-NEXT:    loop:
+; CHECK-NEXT:      Report: cannot identify array bounds
+; CHECK-NEXT:      Dependences:
+; CHECK-NEXT:      Run-time memory checks:
+; CHECK-NEXT:      Grouped accesses:
+; CHECK-EMPTY:
+; CHECK-NEXT:      Non vectorizable stores to invariant address were not found in loop.
+; CHECK-NEXT:      SCEV assumptions:
+; CHECK-NEXT:      {0,+,1}<%loop> Added Flags: <nusw>
+; CHECK-NEXT:      {%v,+,4}<%loop> Added Flags: <nusw>
+; CHECK-NEXT:      {%w,+,4}<%loop> Added Flags: <nusw>
+; CHECK-EMPTY:
+; CHECK-NEXT:      Expressions re-written:
+;
+entry:
+   br label %loop
+
+loop:
+   %iv = phi i64 [ 0, %entry ], [ %iv.next, %loop ]
+   %gep.v = getelementptr {i16, i16}, ptr %v, i64 %iv
+   store i16 0, ptr %gep.v, align 1
+   %gep.w = getelementptr i32, ptr addrspace(3) %w, i64 %iv
+   store i32 0, ptr addrspace(3) %gep.w, align 1
+   %iv.next = add i64 %iv, 1
+   %iv.i32 = trunc i64 %iv to i32
+   %.not = icmp ult i32 %N, %iv.i32
+   br i1 %.not, label %exit, label %loop
+
+ exit:
+   ret void
+ }

[nikic]

LGTM, I think bailout out on different address spaces is fine.

[nikic]

Suggested change

	if (Start->getType()->getPointerAddressSpace() !=
	OpStart->getType()->getPointerAddressSpace())
	if (Start->getType() != OpStart->getType())

[fhahn]

LGTM, thanks

[fhahn]

nit: might be worth adding a comment here to say that this rejects pointers in different address spaces?

[llvm-ci]

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-4 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/19268

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang-Unit :: ./AllClangUnitTests/36/51' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests-Clang-Unit-6169-36-51.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=51 GTEST_SHARD_INDEX=36 /Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests
--

Script:
--
/Users/buildbot/buildbot-root/aarch64-darwin/build/tools/clang/unittests/./AllClangUnitTests --gtest_filter=TimeProfilerTest.ConstantEvaluationCxx20
--
/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/unittests/Support/TimeProfilerTest.cpp:238: Failure
Expected equality of these values:
  R"(
Frontend (test.cc)
| ParseDeclarationOrFunctionDefinition (test.cc:2:1)
| ParseDeclarationOrFunctionDefinition (test.cc:6:1)
| | ParseFunctionDefinition (slow_func)
| | | EvaluateAsRValue (<test.cc:8:21>)
| | | EvaluateForOverflow (<test.cc:8:21, col:25>)
| | | EvaluateForOverflow (<test.cc:8:30, col:32>)
| | | EvaluateAsRValue (<test.cc:9:14>)
| | | EvaluateForOverflow (<test.cc:9:9, col:14>)
| | | isPotentialConstantExpr (slow_namespace::slow_func)
| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)
| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)
| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)
| ParseDeclarationOrFunctionDefinition (test.cc:16:1)
| | ParseFunctionDefinition (slow_test)
| | | EvaluateAsInitializer (slow_value)
| | | EvaluateAsConstantExpr (<test.cc:17:33, col:59>)
| | | EvaluateAsConstantExpr (<test.cc:18:11, col:37>)
| ParseDeclarationOrFunctionDefinition (test.cc:22:1)
| | EvaluateAsConstantExpr (<test.cc:23:31, col:57>)
| | EvaluateAsRValue (<test.cc:22:14, line:23:58>)
| ParseDeclarationOrFunctionDefinition (test.cc:25:1)
| | EvaluateAsInitializer (slow_init_list)
| PerformPendingInstantiations
)"
    Which is: "\nFrontend (test.cc)\n| ParseDeclarationOrFunctionDefinition (test.cc:2:1)\n| ParseDeclarationOrFunctionDefinition (test.cc:6:1)\n| | ParseFunctionDefinition (slow_func)\n| | | EvaluateAsRValue (<test.cc:8:21>)\n| | | EvaluateForOverflow (<test.cc:8:21, col:25>)\n| | | EvaluateForOverflow (<test.cc:8:30, col:32>)\n| | | EvaluateAsRValue (<test.cc:9:14>)\n| | | EvaluateForOverflow (<test.cc:9:9, col:14>)\n| | | isPotentialConstantExpr (slow_namespace::slow_func)\n| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)\n| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)\n| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)\n| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)\n| ParseDeclarationOrFunctionDefinition (test.cc:16:1)\n| | ParseFunctionDefinition (slow_test)\n| | | EvaluateAsInitializer (slow_value)\n| | | EvaluateAsConstantExpr (<test.cc:17:33, col:59>)\n| | | EvaluateAsConstantExpr (<test.cc:18:11, col:37>)\n| ParseDeclarationOrFunctionDefinition (test.cc:22:1)\n| | EvaluateAsConstantExpr (<test.cc:23:31, col:57>)\n| | EvaluateAsRValue (<test.cc:22:14, line:23:58>)\n| ParseDeclarationOrFunctionDefinition (test.cc:25:1)\n| | EvaluateAsInitializer (slow_init_list)\n| PerformPendingInstantiations\n"
  buildTraceGraph(Json)
    Which is: "\nFrontend (test.cc)\n| ParseDeclarationOrFunctionDefinition (test.cc:2:1)\n| ParseDeclarationOrFunctionDefinition (test.cc:6:1)\n| | ParseFunctionDefinition (slow_func)\n| | | EvaluateAsRValue (<test.cc:8:21>)\n| | | EvaluateForOverflow (<test.cc:8:21, col:25>)\n| | | EvaluateForOverflow (<test.cc:8:30, col:32>)\n| | | EvaluateAsRValue (<test.cc:9:14>)\n| | | EvaluateForOverflow (<test.cc:9:9, col:14>)\n| | | isPotentialConstantExpr (slow_namespace::slow_func)\n| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)\n| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)\n| | | EvaluateAsBooleanCondition (<test.cc:8:21, col:25>)\n| | | | EvaluateAsRValue (<test.cc:8:21, col:25>)\n| ParseDeclarationOrFunctionDefinition (test.cc:16:1)\n| | ParseFunctionDefinition (slow_test)\n| | | EvaluateAsInitializer (slow_value)\n| | | EvaluateAsConstantExpr (<test.cc:17:33, col:59>)\n| | | EvaluateAsConstantExpr (<test.cc:18:11, col:37>)\n| ParseDeclarationOrFunctionDefinition (test.cc:22:1)\n| | EvaluateAsConstantExpr (<test.cc:23:31, col:57>)\n| | EvaluateAsRValue (<test.cc:22:14, line:23:58>)\n| ParseDeclarationOrFunctionDefinition (test.cc:25:1)\n| | EvaluateAsInitializer (slow_init_list)\n| | PerformPendingInstantiations\n"
With diff:
@@ -24,3 +24,3 @@
 | ParseDeclarationOrFunctionDefinition (test.cc:25:1)
 | | EvaluateAsInitializer (slow_init_list)
-| PerformPendingInstantiations\n
+| | PerformPendingInstantiations\n


...