[PATCH] D89031: [SVE] Add support to vectorize_width loop pragma for scalable vectors

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:144
+      assert(ValueExpr && "Attribute must have a valid value expression.");
+      if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+        return nullptr;
----------------
sdesmalen wrote:
> david-arm wrote:
> > fhahn wrote:
> > > Is there a way to only accept `fixed_width/scalable` for targets that support it? Not sure if we have enough information here, but we might be able to reject it eg per target basis or something
> > Hi @fhahn, I think if possible we'd prefer not to reject scalable vectors at this point. Theoretically there is no reason why we can't perform scalable vectorisation for targets that don't have hardware support for scalable vectors. In this case it simply means that vscale is 1. If you want we could add some kind of opt-remark in the vectoriser that says something like "target does not support scalable vectors, vectorising for vscale=1"?
> I agree with @david-arm that we shouldn't prevent this in the front-end. Even if the architecture may not support scalable vectors natively, there may still be reasons to want to create scalable vectors in IR, for example to have more portable IR.
Hm, I am just a bit worried that it might be a bit confusing to users that do not know what scalable vectors are (it is obvious when knowing all about SVE, but I would assume most people don't necessarily know what this means). I guess that is not the biggest deal, as long `vectorize_width(X, scalable)` works for every target.

>  Even if the architecture may not support scalable vectors natively, there may still be reasons to want to create scalable vectors in IR, for example to have more portable IR.

Sure, but there are so many other target-specific things encoded that make the IR really un-portable between targets. Granted, it is not impossible to convert IR between some architectures (as in arm64_32)


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

https://reviews.llvm.org/D89031