-
Notifications
You must be signed in to change notification settings - Fork 15.7k
[LLVM][LangRef] Redefine out-of-range stepvector values as being truncated. #173494
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
[LLVM][LangRef] Redefine out-of-range stepvector values as being truncated. #173494
Conversation
…cated. The LangRef current defines out-of-range stepvector values as poison. This property is at odds with both the expansion used for fixed-length vectors and the equivalent ISD node, both of which implicitly truncate out-of-range values. NOTE: In order to keep the PR mostly NFC I would like to defer the follow extensions to seperate PRs. 1) The new definition means the "8-bit" restriction can be lifted because that only existed due to problematic cases like `<vscale x n x i1> stepvector()`, which by definition is mostly poison. Defering because I'm unsure of the code generation support for smaller types, as a minimum we're missing test coverage. 2) The instcombine can fire in many more case, and the current constant handling can be a simplification rather than a combine.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Paul Walker (paulwalker-arm) ChangesThe LangRef current defines out-of-range stepvector values as poison. This property is at odds with both the expansion used for fixed-length vectors and the equivalent ISD node, both of which implicitly truncate out-of-range values. NOTE: In order to keep the PR mostly NFC I would like to defer the follow extensions to seperate PRs.
Full diff: https://github.com/llvm/llvm-project/pull/173494.diff 3 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index d99280f05e73f..5b462b87acb0f 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -20828,7 +20828,7 @@ of integers whose elements contain a linear sequence of values starting from 0
with a step of 1. This intrinsic can only be used for vectors with integer
elements that are at least 8 bits in size. If the sequence value exceeds
the allowed limit for the element type then the result for that lane is
-a poison value.
+truncated.
These intrinsics work for both fixed and scalable vectors. While this intrinsic
supports all vector types, the recommended way to express this operation for
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
index 98e2d9ebe4fc2..f5e8d341e3493 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
@@ -448,7 +448,7 @@ Instruction *InstCombinerImpl::visitExtractElementInst(ExtractElementInst &EI) {
if (IndexC->getValue().getActiveBits() <= BitWidth)
Idx = ConstantInt::get(Ty, IndexC->getValue().zextOrTrunc(BitWidth));
else
- Idx = PoisonValue::get(Ty);
+ return nullptr;
return replaceInstUsesWith(EI, Idx);
}
}
diff --git a/llvm/test/Transforms/InstCombine/vscale_extractelement.ll b/llvm/test/Transforms/InstCombine/vscale_extractelement.ll
index 9ac8a92abb689..ec58307a253d1 100644
--- a/llvm/test/Transforms/InstCombine/vscale_extractelement.ll
+++ b/llvm/test/Transforms/InstCombine/vscale_extractelement.ll
@@ -214,12 +214,15 @@ entry:
ret i64 %1
}
-; Check that poison is returned when the extracted element has wrapped.
+; TODO: stepvector now wraps rather than poisons elements when the value does
+; not fit, so this should return 0.
define i8 @ext_lane256_from_stepvec() {
; CHECK-LABEL: @ext_lane256_from_stepvec(
; CHECK-NEXT: entry:
-; CHECK-NEXT: ret i8 poison
+; CHECK-NEXT: [[TMP0:%.*]] = call <vscale x 512 x i8> @llvm.stepvector.nxv512i8()
+; CHECK-NEXT: [[TMP1:%.*]] = extractelement <vscale x 512 x i8> [[TMP0]], i64 256
+; CHECK-NEXT: ret i8 [[TMP1]]
;
entry:
%0 = call <vscale x 512 x i8> @llvm.stepvector.nxv512i8()
|
| unsigned BitWidth = Ty->getIntegerBitWidth(); | ||
| Value *Idx; | ||
| // Return index when its value does not exceed the allowed limit | ||
| // for the element type of the vector, otherwise return undefined. |
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.
This comment is outdated now.
Why not drop the if entirely here instead of keeping the nullptr return?
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.
Why not drop the if entirely here instead of keeping the nullptr return?
That's point two in the message. This whole functionality belongs in simplifyExtractElementInst, so I figured it's better to do the work once in a separate PR?
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'm fine with that. Maybe worth noting that InstCombine could handle the generalized case of a variable index extract (where a zext/trunc instruction may have to be generated).
nikic
left a comment
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
topperc
left a comment
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. The RISC-V instruction vid.v we use for stepvector has the truncate behavior.
fhahn
left a comment
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 thanks
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/19539 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/43267 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/35525 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/14985 Here is the relevant piece of the build log for the reference |
LV can create step vectors that wrap around, e.g. `step-vector i1` with VF>2. Allow truncation when creating the vector constant to avoid an assertion failure with #171456. After #173494 the definition of the llvm.stepvector intrinsic has been changed to make it have wrapping semantics, so the semantics for the fixed and scalable case match now.
…#173229) LV can create step vectors that wrap around, e.g. `step-vector i1` with VF>2. Allow truncation when creating the vector constant to avoid an assertion failure with llvm/llvm-project#171456. After llvm/llvm-project#173494 the definition of the llvm.stepvector intrinsic has been changed to make it have wrapping semantics, so the semantics for the fixed and scalable case match now.
…cated. (llvm#173494) The LangRef current defines out-of-range stepvector values as poison. This property is at odds with both the expansion used for fixed-length vectors and the equivalent ISD node, both of which implicitly truncate out-of-range values.
LV can create step vectors that wrap around, e.g. `step-vector i1` with VF>2. Allow truncation when creating the vector constant to avoid an assertion failure with llvm#171456. After llvm#173494 the definition of the llvm.stepvector intrinsic has been changed to make it have wrapping semantics, so the semantics for the fixed and scalable case match now.
The LangRef current defines out-of-range stepvector values as poison. This property is at odds with both the expansion used for fixed-length vectors and the equivalent ISD node, both of which implicitly truncate out-of-range values.
NOTE: In order to keep the PR mostly NFC I would like to defer the follow extensions to seperate PRs.
The new definition means the "8-bit" restriction can be lifted
because that only existed due to problematic cases like
<vscale x n x i1> stepvector(), which by definition is mostly poison.Defering because I'm unsure of the code generation support for
smaller types, as a minimum we're missing test coverage.
The instcombine can fire in many more case, and the current constant
handling can be a simplification rather than a combine.