Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BasicAA] Check for Overflow using vscale_range #81144

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

davemgreen
Copy link
Collaborator

This extends #80818 when IsNSW is lost (possibly due to looking through multiple GEPs), to check the vscale_range for an access that will not overflow even with the maximum range.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 8, 2024

@llvm/pr-subscribers-llvm-analysis

Author: David Green (davemgreen)

Changes

This extends #80818 when IsNSW is lost (possibly due to looking through multiple GEPs), to check the vscale_range for an access that will not overflow even with the maximum range.


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/BasicAliasAnalysis.cpp (+17-7)
  • (modified) llvm/test/Analysis/BasicAA/vscale.ll (+20-2)
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index ae31814bb06735..38dba376ae6c55 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -1173,7 +1173,7 @@ AliasResult BasicAAResult::aliasGEP(
   // VScale Alias Analysis - Given one scalable offset between accesses and a
   // scalable typesize, we can divide each side by vscale, treating both values
   // as a constant. We prove that Offset/vscale >= TypeSize/vscale.
-  if (DecompGEP1.VarIndices.size() == 1 && DecompGEP1.VarIndices[0].IsNSW &&
+  if (DecompGEP1.VarIndices.size() == 1 &&
       DecompGEP1.VarIndices[0].Val.TruncBits == 0 &&
       DecompGEP1.Offset.isZero() &&
       PatternMatch::match(DecompGEP1.VarIndices[0].Val.V,
@@ -1183,12 +1183,22 @@ AliasResult BasicAAResult::aliasGEP(
         ScalableVar.IsNegated ? -ScalableVar.Scale : ScalableVar.Scale;
     LocationSize VLeftSize = Scale.isNegative() ? V1Size : V2Size;
 
-    // Note that we do not check that the typesize is scalable, as vscale >= 1
-    // so noalias still holds so long as the dependency distance is at least as
-    // big as the typesize.
-    if (VLeftSize.hasValue() &&
-        Scale.uge(VLeftSize.getValue().getKnownMinValue()))
-      return AliasResult::NoAlias;
+    // Check if the offset is known to not overflow, if it does then attempt to
+    // prove it with the known values of vscale_range.
+    bool Overflows = !DecompGEP1.VarIndices[0].IsNSW;
+    if (Overflows) {
+      ConstantRange CR = getVScaleRange(&F, Scale.getBitWidth());
+      (void)CR.getSignedMax().smul_ov(Scale, Overflows);
+    }
+
+    if (!Overflows) {
+      // Note that we do not check that the typesize is scalable, as vscale >= 1
+      // so noalias still holds so long as the dependency distance is at least as
+      // big as the typesize.
+      if (VLeftSize.hasValue() &&
+          Scale.uge(VLeftSize.getValue().getKnownMinValue()))
+        return AliasResult::NoAlias;
+    }
   }
 
   // Bail on analysing scalable LocationSize
diff --git a/llvm/test/Analysis/BasicAA/vscale.ll b/llvm/test/Analysis/BasicAA/vscale.ll
index ce0c6f145d1c88..15a2d01c88d3b6 100644
--- a/llvm/test/Analysis/BasicAA/vscale.ll
+++ b/llvm/test/Analysis/BasicAA/vscale.ll
@@ -458,11 +458,29 @@ define void @vscale_v1v2types(ptr %p) {
   ret void
 }
 
+; CHECK-LABEL: onevscale
+; CHECK-DAG:   MustAlias:    <vscale x 4 x i32>* %vp161, <vscale x 4 x i32>* %vp162
+; CHECK-DAG:   NoAlias:      <vscale x 4 x i32>* %vp161, <vscale x 4 x i32>* %vp161b
+; CHECK-DAG:   NoAlias:      <vscale x 4 x i32>* %vp161b, <vscale x 4 x i32>* %vp162
+define void @onevscale(ptr %p) vscale_range(1,16) {
+  %v1 = call i64 @llvm.vscale.i64()
+  %vp1 = mul nsw i64 %v1, 16
+  %vp2 = mul nsw i64 %v1, 16
+  %vp3 = mul nsw i64 %v1, 17
+  %vp161 = getelementptr i8, ptr %p, i64 %vp1
+  %vp162 = getelementptr i8, ptr %p, i64 %vp2
+  %vp161b = getelementptr i8, ptr %vp161, i64 %vp3
+  load <vscale x 4 x i32>, ptr %vp161
+  load <vscale x 4 x i32>, ptr %vp162
+  load <vscale x 4 x i32>, ptr %vp161b
+  ret void
+}
+
 ; CHECK-LABEL: twovscales
 ; CHECK-DAG:   MayAlias:     <vscale x 4 x i32>* %vp161, <vscale x 4 x i32>* %vp162
-; CHECK-DAG:   MayAlias:     <vscale x 4 x i32>* %vp161, <vscale x 4 x i32>* %vp161b
+; CHECK-DAG:   NoAlias:      <vscale x 4 x i32>* %vp161, <vscale x 4 x i32>* %vp161b
 ; CHECK-DAG:   MayAlias:     <vscale x 4 x i32>* %vp161b, <vscale x 4 x i32>* %vp162
-define void @twovscales(ptr %p) {
+define void @twovscales(ptr %p) vscale_range(1,16) {
   %v1 = call i64 @llvm.vscale.i64()
   %v2 = call i64 @llvm.vscale.i64()
   %vp1 = mul nsw i64 %v1, 16

Copy link

github-actions bot commented Feb 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

// so noalias still holds so long as the dependency distance is at least
// as big as the typesize.
if (VLeftSize.hasValue() &&
Scale.uge(VLeftSize.getValue().getKnownMinValue()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this when reviewing the original patch: Isn't this Scale supposed to be abs(Scale)? I don't think this does the right thing for negative scales right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh Yep. I'll put a patch together with a test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the Scale in this patch as it is used in a signed multiply. Let me know if you think that should change.

davemgreen added a commit to davemgreen/llvm-project that referenced this pull request Feb 8, 2024
This is a fix for llvm#80818, as pointed out in llvm#81144 it should be checking the
abs of Scale. The added test changes from NoAlias to MayAlias.
davemgreen added a commit that referenced this pull request Feb 9, 2024
This is a fix for #80818, as pointed out in #81144 it should be checking
the abs of Scale. The added test changes from NoAlias to MayAlias.
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This extends llvm#80818 when IsNSW is lost (possibly due to looking through
multiple GEPs), to check the vscale_range for an access that will not overflow
even with the maximum range.
@davemgreen davemgreen merged commit 9d8a236 into llvm:main Feb 12, 2024
3 of 4 checks passed
@davemgreen davemgreen deleted the gh-aa-nswfromrange branch February 12, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants