-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[clang][CodeGen] Fix metadata when vectorization is disabled by pragma #135163
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
Changes from 5 commits
79186af
aed6f10
98bee49
0e60403
16b6d51
375dc8f
6d68a79
32a6d9c
2989929
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5237,8 +5237,8 @@ width/count of the set of target architectures supported by your application. | |
... | ||
} | ||
|
||
Specifying a width/count of 1 disables the optimization, and is equivalent to | ||
``vectorize(disable)`` or ``interleave(disable)``. | ||
Specifying a *non-scalable* width/count of 1 disables the optimization, and is | ||
|
||
equivalent to ``vectorize(disable)`` or ``interleave(disable)``. | ||
|
||
Vector predication is enabled by ``vectorize_predicate(enable)``, for example: | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -200,7 +200,14 @@ LoopInfo::createLoopVectorizeMetadata(const LoopAttributes &Attrs, | |
LLVMContext &Ctx = Header->getContext(); | ||
|
||
std::optional<bool> Enabled; | ||
if (Attrs.VectorizeEnable == LoopAttributes::Disable) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps this condition has never been met. |
||
|
||
// Vectorization is disabled if: | ||
// 1) it is disabled explicitly, or | ||
// 2) it is implied when vectorize.width is set to 1 and scalable | ||
// vectorization is not specified explicitly. | ||
if (Attrs.VectorizeEnable == LoopAttributes::Disable || | ||
(Attrs.VectorizeWidth == 1 && | ||
Attrs.VectorizeScalable != LoopAttributes::Enable)) | ||
Enabled = false; | ||
else if (Attrs.VectorizeEnable != LoopAttributes::Unspecified || | ||
Attrs.VectorizePredicateEnable != LoopAttributes::Unspecified || | ||
|
@@ -643,9 +650,7 @@ void LoopInfoStack::push(BasicBlock *Header, clang::ASTContext &Ctx, | |
case LoopHintAttr::Disable: | ||
switch (Option) { | ||
case LoopHintAttr::Vectorize: | ||
// Disable vectorization by specifying a width of 1. | ||
setVectorizeWidth(1); | ||
setVectorizeScalable(LoopAttributes::Unspecified); | ||
setVectorizeEnable(false); | ||
|
||
break; | ||
case LoopHintAttr::Interleave: | ||
// Disable interleaving by speciyfing a count of 1. | ||
|
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.
Should be read as:
Specifying a vector width of 1 disables vectorization, and is equivalent to
vectorize(disable)
. Specifying a interleave count of 1 disables interlaving, and is equivalent tointerleave(disable)
.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 understand now, thanks (IMHO this could be a bit confusing).
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've been thinking about this for a while, and now I think it triggers ambiguity in the pragma interpretation if we consider
vectorize(disable)
andvectorize_width(1)
to be equivalent.vectorize(disable) vectorize_width(scalable)
is an example of such a case. The document says:In this perspective, I think it is correct to ignore
vectorize_width(scalable)
and disable vectorization, since it falls under "transformations option pragmas implying that transformation". However, it also makes sense to considervectorize(disable) vectorize_width(scalable)
asvectorize_width(1, scalable)
ifvectorize(disable)
is equivalent tovectorize_width(1)
. In this case, we should process vectorization, as mentioned in the previous review comment.I think we should stop interpreting
vectorize(disable)
andvectorize_width(1)
as equivalent, but it might break some compatibilities. So, what do you think?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 in princeiple I agree, but considereing that
vectorize(disable) vectorize_width(scalable)
is being rejected, where is the ambiguity? Isvectorize_width(1) vectorize_width(scalable)
the issue? I don't think that it should be interpreted asvectorize_width(1, scalable)
, it is just conflicting information for the same setting.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.
Correct. The previous my comment were written before noticing the fact that
vectorize(disable) vectorize_width(scalable)
is being rejected. Sorry for confusion, I also don't think that there are any ambiguities.