[PATCH] D69628: [Clang] Pragma vectorize_width() implies vectorize(enable), take 3
Sjoerd Meijer via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 13 09:04:57 PST 2019
SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.
================
Comment at: clang/lib/CodeGen/CGLoopInfo.cpp:302-306
+ // Imply vectorize.enable when it is not already disabled/enabled.
+ Args.push_back(
+ MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.vectorize.enable"),
+ ConstantAsMetadata::get(ConstantInt::get(
+ llvm::Type::getInt1Ty(Ctx), 1))}));
----------------
Meinersbur wrote:
> [serious] Why not reusing the `Args.push_back` code from above? I think it is important `vectorize_predicate` and `vectorize_width` (and ever additional property we introduce in the future) the same way. IMHO everything else becomes more and more confusing.
> I have the following in mind:
>
> ```
> if (Attrs.VectorizeEnable != LoopAttributes::Unspecified ||
> IsVectorPredicateEnabled || Attrs.VectorizeWidth > 1) {
> auto AttrVal = Attrs.VectorizeEnable != LoopAttributes::Disable;
> Args.push_back(..., ConstantInt::get(AttrVal));
> }
> ```
>
No worries, and thanks for looking again! I was a bit reluctant to touch that piece of logic (and actually thought this if-elseif was not too bad and explicit in identifying the different cases), but yeah, what you suggest make sense, so will address this soon.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69628/new/
https://reviews.llvm.org/D69628
More information about the cfe-commits
mailing list