[PatternMatch] Do not accept undef elements in m_AllOnes() and friends #88217
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-ir Author: Nikita Popov (nikic) ChangesChange all the cstval_pred_ty based PatternMatch helpers (things like m_AllOnes and m_Zero) to only allow poison elements inside vector splats, not undef elements. Historically, we used to represent non-demanded elements in vectors using undef. Nowadays, we use poison instead. As such, I believe that support for undef in vector splats is no longer useful. At the same time, while poison splat elements are pretty much always safe to ignore, this is not generally the case for undef elements. We have existing miscompiles in our tests due to this (see the masked-merge-*.ll tests changed here) and it's easy to miss such cases in the future, now that we write tests using poison instead of undef elements. I think overall, keeping support for undef elements no longer makes sense, and we should drop it. Once this is done consistently, I think we may also consider allowing poison in m_APInt by default, as doing that change is much less risky than doing the same with undef. This PR is a stub for discussion -- this change has more than a hundred test failures in InstCombine alone, and I don't want to update all of them before we reach a consensus. The abs-1.ll test is a representative example of how I would update most tests: By replacing undef with poison. I don't think there is value in retaining the undef test coverage anymore at this point. Full diff: https://github.com/llvm/llvm-project/pull/88217.diff 5 Files Affected:
diff --git a/llvm/include/llvm/IR/PatternMatch.h b/llvm/include/llvm/IR/PatternMatch.h
index 92cb79d54afc29..c5ea14af63f862 100644
--- a/llvm/include/llvm/IR/PatternMatch.h
+++ b/llvm/include/llvm/IR/PatternMatch.h
@@ -345,7 +345,7 @@ template <int64_t Val> inline constantint_match<Val> m_ConstantInt() {
/// This helper class is used to match constant scalars, vector splats,
/// and fixed width vectors that satisfy a specified predicate.
-/// For fixed width vector constants, undefined elements are ignored.
+/// For fixed width vector constants, poison elements are ignored.
template <typename Predicate, typename ConstantVal>
struct cstval_pred_ty : public Predicate {
template <typename ITy> bool match(ITy *V) {
@@ -364,19 +364,19 @@ struct cstval_pred_ty : public Predicate {
// Non-splat vector constant: check each element for a match.
unsigned NumElts = FVTy->getNumElements();
assert(NumElts != 0 && "Constant vector with no elements?");
- bool HasNonUndefElements = false;
+ bool HasNonPoisonElements = false;
for (unsigned i = 0; i != NumElts; ++i) {
Constant *Elt = C->getAggregateElement(i);
if (!Elt)
return false;
- if (isa<UndefValue>(Elt))
+ if (isa<PoisonValue>(Elt))
continue;
auto *CV = dyn_cast<ConstantVal>(Elt);
if (!CV || !this->isValue(CV->getValue()))
return false;
- HasNonUndefElements = true;
+ HasNonPoisonElements = true;
}
- return HasNonUndefElements;
+ return HasNonPoisonElements;
}
}
return false;
diff --git a/llvm/test/Transforms/InstCombine/abs-1.ll b/llvm/test/Transforms/InstCombine/abs-1.ll
index 7355c560c820b2..32bd7a37053ed6 100644
--- a/llvm/test/Transforms/InstCombine/abs-1.ll
+++ b/llvm/test/Transforms/InstCombine/abs-1.ll
@@ -63,14 +63,14 @@ define <2 x i8> @abs_canonical_2(<2 x i8> %x) {
ret <2 x i8> %abs
}
-; Even if a constant has undef elements.
+; Even if a constant has poison elements.
-define <2 x i8> @abs_canonical_2_vec_undef_elts(<2 x i8> %x) {
-; CHECK-LABEL: @abs_canonical_2_vec_undef_elts(
+define <2 x i8> @abs_canonical_2_vec_poison_elts(<2 x i8> %x) {
+; CHECK-LABEL: @abs_canonical_2_vec_poison_elts(
; CHECK-NEXT: [[ABS:%.*]] = call <2 x i8> @llvm.abs.v2i8(<2 x i8> [[X:%.*]], i1 false)
; CHECK-NEXT: ret <2 x i8> [[ABS]]
;
- %cmp = icmp sgt <2 x i8> %x, <i8 undef, i8 -1>
+ %cmp = icmp sgt <2 x i8> %x, <i8 poison, i8 -1>
%neg = sub <2 x i8> zeroinitializer, %x
%abs = select <2 x i1> %cmp, <2 x i8> %x, <2 x i8> %neg
ret <2 x i8> %abs
@@ -208,15 +208,15 @@ define <2 x i8> @nabs_canonical_2(<2 x i8> %x) {
ret <2 x i8> %abs
}
-; Even if a constant has undef elements.
+; Even if a constant has poison elements.
-define <2 x i8> @nabs_canonical_2_vec_undef_elts(<2 x i8> %x) {
-; CHECK-LABEL: @nabs_canonical_2_vec_undef_elts(
+define <2 x i8> @nabs_canonical_2_vec_poison_elts(<2 x i8> %x) {
+; CHECK-LABEL: @nabs_canonical_2_vec_poison_elts(
; CHECK-NEXT: [[TMP1:%.*]] = call <2 x i8> @llvm.abs.v2i8(<2 x i8> [[X:%.*]], i1 false)
; CHECK-NEXT: [[ABS:%.*]] = sub <2 x i8> zeroinitializer, [[TMP1]]
; CHECK-NEXT: ret <2 x i8> [[ABS]]
;
- %cmp = icmp sgt <2 x i8> %x, <i8 -1, i8 undef>
+ %cmp = icmp sgt <2 x i8> %x, <i8 -1, i8 poison>
%neg = sub <2 x i8> zeroinitializer, %x
%abs = select <2 x i1> %cmp, <2 x i8> %neg, <2 x i8> %x
ret <2 x i8> %abs
diff --git a/llvm/test/Transforms/InstCombine/masked-merge-add.ll b/llvm/test/Transforms/InstCombine/masked-merge-add.ll
index f655153108a436..0484369e99d6a5 100644
--- a/llvm/test/Transforms/InstCombine/masked-merge-add.ll
+++ b/llvm/test/Transforms/InstCombine/masked-merge-add.ll
@@ -51,7 +51,7 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 undef, i32 -1>
; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
-; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: [[RET:%.*]] = add <3 x i32> [[AND]], [[AND1]]
; CHECK-NEXT: ret <3 x i32> [[RET]]
;
%and = and <3 x i32> %x, %m
@@ -61,6 +61,21 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
ret <3 x i32> %ret
}
+define <3 x i32> @p_vec_poison(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m) {
+; CHECK-LABEL: @p_vec_poison(
+; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
+; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 poison, i32 -1>
+; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
+; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: ret <3 x i32> [[RET]]
+;
+ %and = and <3 x i32> %x, %m
+ %neg = xor <3 x i32> %m, <i32 -1, i32 poison, i32 -1>
+ %and1 = and <3 x i32> %neg, %y
+ %ret = add <3 x i32> %and, %and1
+ ret <3 x i32> %ret
+}
+
; ============================================================================ ;
; Constant mask.
; ============================================================================ ;
diff --git a/llvm/test/Transforms/InstCombine/masked-merge-or.ll b/llvm/test/Transforms/InstCombine/masked-merge-or.ll
index b49ec07706e284..0531a532fc7e0a 100644
--- a/llvm/test/Transforms/InstCombine/masked-merge-or.ll
+++ b/llvm/test/Transforms/InstCombine/masked-merge-or.ll
@@ -51,7 +51,7 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 undef, i32 -1>
; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
-; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: [[RET:%.*]] = or <3 x i32> [[AND]], [[AND1]]
; CHECK-NEXT: ret <3 x i32> [[RET]]
;
%and = and <3 x i32> %x, %m
@@ -61,6 +61,21 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
ret <3 x i32> %ret
}
+define <3 x i32> @p_vec_poison(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m) {
+; CHECK-LABEL: @p_vec_poison(
+; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
+; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 poison, i32 -1>
+; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
+; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: ret <3 x i32> [[RET]]
+;
+ %and = and <3 x i32> %x, %m
+ %neg = xor <3 x i32> %m, <i32 -1, i32 poison, i32 -1>
+ %and1 = and <3 x i32> %neg, %y
+ %ret = or <3 x i32> %and, %and1
+ ret <3 x i32> %ret
+}
+
; ============================================================================ ;
; Constant mask.
; ============================================================================ ;
diff --git a/llvm/test/Transforms/InstCombine/masked-merge-xor.ll b/llvm/test/Transforms/InstCombine/masked-merge-xor.ll
index a6d201be68cee5..74cc7625aebff5 100644
--- a/llvm/test/Transforms/InstCombine/masked-merge-xor.ll
+++ b/llvm/test/Transforms/InstCombine/masked-merge-xor.ll
@@ -51,7 +51,7 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 undef, i32 -1>
; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
-; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: [[RET:%.*]] = xor <3 x i32> [[AND]], [[AND1]]
; CHECK-NEXT: ret <3 x i32> [[RET]]
;
%and = and <3 x i32> %x, %m
@@ -61,6 +61,21 @@ define <3 x i32> @p_vec_undef(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m)
ret <3 x i32> %ret
}
+define <3 x i32> @p_vec_poison(<3 x i32> %x, <3 x i32> %y, <3 x i32> noundef %m) {
+; CHECK-LABEL: @p_vec_poison(
+; CHECK-NEXT: [[AND:%.*]] = and <3 x i32> [[X:%.*]], [[M:%.*]]
+; CHECK-NEXT: [[NEG:%.*]] = xor <3 x i32> [[M]], <i32 -1, i32 poison, i32 -1>
+; CHECK-NEXT: [[AND1:%.*]] = and <3 x i32> [[NEG]], [[Y:%.*]]
+; CHECK-NEXT: [[RET:%.*]] = or disjoint <3 x i32> [[AND]], [[AND1]]
+; CHECK-NEXT: ret <3 x i32> [[RET]]
+;
+ %and = and <3 x i32> %x, %m
+ %neg = xor <3 x i32> %m, <i32 -1, i32 poison, i32 -1>
+ %and1 = and <3 x i32> %neg, %y
+ %ret = xor <3 x i32> %and, %and1
+ ret <3 x i32> %ret
+}
+
; ============================================================================ ;
; Constant mask.
; ============================================================================ ;
|
nunoplopes
approved these changes
Apr 10, 2024
dtcxzyw
approved these changes
Apr 10, 2024
There was a problem hiding this comment.
LGTM. BTW, can we drop the call to Constant::replaceUndefsWith in InstCombine after this patch lands?
See also https://github.com/llvm/llvm-project/pull/85772/files#r1530496643.
|
@dtcxzyw It will take more changes to get there, but yes, we should definitely get rid of such replaceUndefsWith calls. |
nikic
force-pushed
the
patternmatch-undef
branch
from
f2d555e
to
31b38e8
Compare
nikic
force-pushed
the
patternmatch-undef
branch
from
31b38e8
to
c4e222f
Compare
|
Okay, this should be ready for another round of review now. I've updated all the affected tests now (mostly by replacing undef -> poison, in some cases by duplicating tests where I thought it had value). I've also made some minor code cleanup while doing the test updates. The main one is that I dropped the m_NotForbidUndef matcher, because all uses of it are fine with poison elements. |
goldsteinn
reviewed
Apr 16, 2024
| ; CHECK-NEXT: ret <3 x i7> <i7 -1, i7 -1, i7 -1> | ||
| ; | ||
| %left = shl <3 x i7> <i7 poison, i7 -1, i7 undef>, %x | ||
| %left = shl <3 x i7> <i7 poison, i7 -1, i7 poison>, %x |
There was a problem hiding this comment.
Was the original transform bad?
There was a problem hiding this comment.
No, the transform was fine as it did not propagate the undef. This test corresponds to the // We could return the original -1 constant to preserve poison elements. comment change in InstructionSimplify.cpp: Now that we only handle poison elements, we could propagate the poison elements. I didn't make the actual change for that in this PR.
Change all the cstval_pred_ty based PatternMatch helpers (things like m_AllOnes and m_Zero) to only allow poison elements inside vector splats, not undef elements. Historically, we used to represent non-demanded elements in vectors using undef. Nowadays, we use poison instead. As such, I believe that support for undef in vector splats is no longer useful. At the same time, while poison splat elements are pretty much always safe to ignore, this is not generally the case for undef elements. We have existing miscompiles in our tests due to this (see the masked-merge-*.ll tests changed here) and it's easy to miss such cases in the future, now that we write tests using poison instead of undef elements. I think overall, keeping support for undef elements no longer makes sense, and we should drop it. Once this is done consistently, I think we may also consider allowing poison in m_APInt by default, as doing that change is much less risky than doing the same with undef. This PR is a stub for discussion -- this change has more than a hundred test failures in InstCombine alone, and I don't want to update all of them before we reach a consensus. The abs-1.ll test is a representative example of how I would update most tests: By replacing undef with poison. I don't think there is value in retaining the undef test coverage anymore at this point.
nikic
force-pushed
the
patternmatch-undef
branch
from
c4e222f
to
f0ecd25
Compare
Co-authored-by: Yingwei Zheng <dtcxzyw@qq.com>
|
Alive2 is complaining about one case (only!): ; Transforms/InstCombine/integer-round-up-pow2-alignment.ll
define <2 x i4> @t18_replacement_0b0001(<2 x i4> %x) {
%x.lowbits = and <2 x i4> %x, { 3, 3 }
%x.lowbits.are.zero = icmp eq <2 x i4> %x.lowbits, { 0, 0 }
%x.biased = add <2 x i4> %x, { 3, 3 }
%x.biased.highbits = and <2 x i4> %x.biased, { 12, poison }
call void @use.v2i4(<2 x i4> %x.biased.highbits)
%x.roundedup = select <2 x i1> %x.lowbits.are.zero, <2 x i4> %x, <2 x i4> %x.biased.highbits
ret <2 x i4> %x.roundedup
}
=>
define <2 x i4> @t18_replacement_0b0001(<2 x i4> %x) {
%x.biased = add <2 x i4> %x, { 3, 3 }
%x.biased.highbits = and <2 x i4> %x.biased, { 12, poison }
call void @use.v2i4(<2 x i4> %x.biased.highbits)
ret <2 x i4> %x.biased.highbits
}
Transformation doesn't verify! (unsound)
ERROR: Target is more poisonous than source
Example:
<2 x i4> %x = < #x0 (0), #x0 (0) >
Source:
<2 x i4> %x.lowbits = < #x0 (0), #x0 (0) >
<2 x i1> %x.lowbits.are.zero = < #x1 (1), #x1 (1) >
<2 x i4> %x.biased = < #x3 (3), #x3 (3) >
<2 x i4> %x.biased.highbits = < #x0 (0), poison >
Function @use.v2i4 returned
<2 x i4> %x.roundedup = < #x0 (0), #x0 (0) >
Target:
<2 x i4> %x.biased = < #x3 (3), #x3 (3) >
<2 x i4> %x.biased.highbits = < #x0 (0), poison >
Function @use.v2i4 returned
Source value: < #x0 (0), #x0 (0) >
Target value: < #x0 (0), poison >Several tests got fixed btw! 🙏 |
goldsteinn
added a commit
to goldsteinn/llvm-project
that referenced
this pull request
Apr 17, 2024
This is a followup to llvm#88217
|
Think for this to be consistent also need #88217 |
nikic
added a commit
to nikic/llvm-project
that referenced
this pull request
Apr 18, 2024
In llvm#88217 a large set of matchers was changed to only accept poison values in splats, but not undef values. This is because we now use poison for non-demanded vector elements, and allowing undef can cause correctness issues. This patch covers the remaining matchers by changing the AllowUndef parameter of getSplatValue() to AllowPoison instead. We also carry out corresponding renames in matchers. As a followup, we may want to change the default for things like m_APInt to m_APIntAllowPoison (as this is much less risky when only allowing poison), but this change doesn't do that. There is one caveat here: We have a single place (X86FixupVectorConstants) which does require handling of vector splats with undefs. This is because this works on backend constant pool entries, which currently still use undef instead of poison for non-demanded elements (because SDAG as a whole does not have an explicit poison representation). As it's just the single use, I've open-coded a getSplatValueAllowUndef() helper there, to discourage use in any other places.
nikic
added a commit
that referenced
this pull request
Apr 18, 2024
We can't directly use the high bits value if it is more poisonous due to poison elements in the masks. This fixes the issue reported in #88217 (comment).
|
@nunoplopes Thanks, this should be fixed by 525d00e. |
nikic
added a commit
that referenced
this pull request
Apr 18, 2024
In #88217 a large set of matchers was changed to only accept poison values in splats, but not undef values. This is because we now use poison for non-demanded vector elements, and allowing undef can cause correctness issues. This patch covers the remaining matchers by changing the AllowUndef parameter of getSplatValue() to AllowPoison instead. We also carry out corresponding renames in matchers. As a followup, we may want to change the default for things like m_APInt to m_APIntAllowPoison (as this is much less risky when only allowing poison), but this change doesn't do that. There is one caveat here: We have a single place (X86FixupVectorConstants) which does require handling of vector splats with undefs. This is because this works on backend constant pool entries, which currently still use undef instead of poison for non-demanded elements (because SDAG as a whole does not have an explicit poison representation). As it's just the single use, I've open-coded a getSplatValueAllowUndef() helper there, to discourage use in any other places.
tmatheson-arm
pushed a commit
to tmatheson-arm/llvm-project
that referenced
this pull request
Apr 22, 2024
llvm#88217) Change all the cstval_pred_ty based PatternMatch helpers (things like m_AllOnes and m_Zero) to only allow poison elements inside vector splats, not undef elements. Historically, we used to represent non-demanded elements in vectors using undef. Nowadays, we use poison instead. As such, I believe that support for undef in vector splats is no longer useful. At the same time, while poison splat elements are pretty much always safe to ignore, this is not generally the case for undef elements. We have existing miscompiles in our tests due to this (see the masked-merge-*.ll tests changed here) and it's easy to miss such cases in the future, now that we write tests using poison instead of undef elements. I think overall, keeping support for undef elements no longer makes sense, and we should drop it. Once this is done consistently, I think we may also consider allowing poison in m_APInt by default, as doing that change is much less risky than doing the same with undef. This change involves a substantial amount of test changes. For most tests, I've just replaced undef with poison, as I don't think there is value in retaining both. For some tests (where the distinction between undef and poison is important), I've duplicated tests.
tmatheson-arm
pushed a commit
to tmatheson-arm/llvm-project
that referenced
this pull request
Apr 22, 2024
We can't directly use the high bits value if it is more poisonous due to poison elements in the masks. This fixes the issue reported in llvm#88217 (comment).
tmatheson-arm
pushed a commit
to tmatheson-arm/llvm-project
that referenced
this pull request
Apr 22, 2024
In llvm#88217 a large set of matchers was changed to only accept poison values in splats, but not undef values. This is because we now use poison for non-demanded vector elements, and allowing undef can cause correctness issues. This patch covers the remaining matchers by changing the AllowUndef parameter of getSplatValue() to AllowPoison instead. We also carry out corresponding renames in matchers. As a followup, we may want to change the default for things like m_APInt to m_APIntAllowPoison (as this is much less risky when only allowing poison), but this change doesn't do that. There is one caveat here: We have a single place (X86FixupVectorConstants) which does require handling of vector splats with undefs. This is because this works on backend constant pool entries, which currently still use undef instead of poison for non-demanded elements (because SDAG as a whole does not have an explicit poison representation). As it's just the single use, I've open-coded a getSplatValueAllowUndef() helper there, to discourage use in any other places.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change all the cstval_pred_ty based PatternMatch helpers (things like m_AllOnes and m_Zero) to only allow poison elements inside vector splats, not undef elements.
Historically, we used to represent non-demanded elements in vectors using undef. Nowadays, we use poison instead. As such, I believe that support for undef in vector splats is no longer useful.
At the same time, while poison splat elements are pretty much always safe to ignore, this is not generally the case for undef elements. We have existing miscompiles in our tests due to this (see the masked-merge-*.ll tests changed here) and it's easy to miss such cases in the future, now that we write tests using poison instead of undef elements.
I think overall, keeping support for undef elements no longer makes sense, and we should drop it. Once this is done consistently, I think we may also consider allowing poison in m_APInt by default, as doing that change is much less risky than doing the same with undef.
This PR is a stub for discussion -- this change has more than a hundred test failures in InstCombine alone, and I don't want to update all of them before we reach a consensus. The abs-1.ll test is a representative example of how I would update most tests: By replacing undef with poison. I don't think there is value in retaining the undef test coverage anymore at this point.