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:
> SjoerdMeijer wrote:
> > 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.
> Sounds like a typical case of "[[ 
> https://en.wikipedia.org/wiki/Technical_debt | technical debt ]]".
yep, but it's been addressed


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69628/new/

https://reviews.llvm.org/D69628



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to