-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[InferAlignment] Fix updating alignment when larger than i32 #160109
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
Conversation
Summary: The changes made in llvm#156057 allows the alignment value to be increased. We assert effectively infinite alignment when the pointer argument is invalid / null. The problem is that for whatever reason the masked load / store functions use i32 for their alignment value which means this gets truncated to zero. Add a special check for this, long term we probably want to just remove this argument entirely.
@llvm/pr-subscribers-llvm-transforms Author: Joseph Huber (jhuber6) ChangesSummary: Add a special check for this, long term we probably want to just remove Full diff: https://github.com/llvm/llvm-project/pull/160109.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/InferAlignment.cpp b/llvm/lib/Transforms/Scalar/InferAlignment.cpp
index b60b15b6c3a2b..995b80396b8af 100644
--- a/llvm/lib/Transforms/Scalar/InferAlignment.cpp
+++ b/llvm/lib/Transforms/Scalar/InferAlignment.cpp
@@ -57,7 +57,8 @@ static bool tryToImproveAlign(
cast<ConstantInt>(II->getArgOperand(AlignOpIdx))->getAlignValue();
Align PrefAlign = DL.getPrefTypeAlign(Type);
Align NewAlign = Fn(PtrOp, OldAlign, PrefAlign);
- if (NewAlign <= OldAlign)
+ if (NewAlign <= OldAlign ||
+ NewAlign.value() > std::numeric_limits<uint32_t>().max())
return false;
Value *V =
diff --git a/llvm/test/Transforms/InferAlignment/masked.ll b/llvm/test/Transforms/InferAlignment/masked.ll
index 1b8d26417d75e..13acf9b50e7e8 100644
--- a/llvm/test/Transforms/InferAlignment/masked.ll
+++ b/llvm/test/Transforms/InferAlignment/masked.ll
@@ -29,6 +29,18 @@ entry:
ret void
}
+define <2 x i32> @null(<2 x i1> %mask, <2 x i32> %val) {
+; CHECK-LABEL: define <2 x i32> @null(
+; CHECK-SAME: <2 x i1> [[MASK:%.*]], <2 x i32> [[VAL:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[MASKED_LOAD:%.*]] = tail call <2 x i32> @llvm.masked.load.v2i32.p0(ptr null, i32 1, <2 x i1> [[MASK]], <2 x i32> [[VAL]])
+; CHECK-NEXT: ret <2 x i32> [[MASKED_LOAD]]
+;
+entry:
+ %masked_load = tail call <2 x i32> @llvm.masked.load.v2f64.p0(ptr null, i32 1, <2 x i1> %mask, <2 x i32> %val)
+ ret <2 x i32> %masked_load
+}
+
declare void @llvm.assume(i1)
declare <2 x i32> @llvm.masked.load.v2i32.p0(ptr, i32, <2 x i1>, <2 x i32>)
declare void @llvm.masked.store.v2i32.p0(<2 x i32>, ptr, i32, <2 x i1>)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Align NewAlign = Fn(PtrOp, OldAlign, PrefAlign); | ||
if (NewAlign <= OldAlign) | ||
if (NewAlign <= OldAlign || | ||
NewAlign.value() > std::numeric_limits<uint32_t>().max()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Value::MaxAlignmentExponent / MaximumAlignment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured this was more straightforward since the problem is that Align is stored as i64
but this argument is i32
, so this makes it clear that it's a size issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arsenm We already use MaxAlignmentExponent -- the problem is that these specific intrinsics don't support the full alignment range (the maximum exponent is one less).
Summary:
The changes made in #156057
allows the alignment value to be increased. We assert effectively
infinite alignment when the pointer argument is invalid / null. The
problem is that for whatever reason the masked load / store functions
use i32 for their alignment value which means this gets truncated to
zero.
Add a special check for this, long term we probably want to just remove
this argument entirely.