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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:939
+def warn_pragma_attribute_scalable_unused : Warning<
+  "ignoring scalable vectorize_width flag due to lack of target support">,
+  InGroup<PragmaClangAttribute>;
----------------
>From what I can see, the vectorize_width flag is not ignored, only the scalable property is. That means this should be:
  'scalable' not supported by the target so assuming 'fixed' instead.


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:148
+      if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable")) {
+        if (!S.Context.getTargetInfo().supportsScalableVectors()) {
+          S.Diag(St->getBeginLoc(), diag::warn_pragma_attribute_scalable_unused);
----------------
If the target does not support scalable vectors, it currently assumes `"fixed"`. If we want to stick with that approach, the diagnostic message should be changed (see my other comment). The alternative is dropping the hint entirely by returning `nullptr` and changing the diagnostic message to say the hint is ignored. I could live with both options. @fhahn do you have a preference here?

nit: to reduce nesting, can you hoist this out one level, e.g.
  if (StateLoc && StateLoc->Ident & ...)
    State = LoopHintAttr::ScalableNumeric;
  else
    State = LoopHintAttr::Numeric;

  if (State == LoopHintAttr::ScalableNumeric &&
      !S.Context.getTargetInfo().supportsScalableVectors()) {
    S.Diag(....);
    State = LoopHintAttr::Numeric;
  }


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

https://reviews.llvm.org/D89031