[PATCH] D146839: [PATCH] [TLI][AArch64] Extend SLEEF vectorized functions mapping with VLA functions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @pawosm01, the title of this patch peaked my interest so I had a look and left some comments. Thanks for working on this.

nit: you'll probably want to remove the `[PATCH]` from the title :)



================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:43
 static void addVariantDeclaration(CallInst &CI, const ElementCount &VF,
+                                  bool GlobalPredicate,
                                   const StringRef VFName) {
----------------
nit: Is there a reason you've named this `GlobalPredicate` instead of just `Predicate` ?


================
Comment at: llvm/lib/Transforms/Utils/InjectTLIMappings.cpp:115
 
   for (ElementCount VF = ElementCount::getFixed(2);
+       ElementCount::isKnownLE(VF, WidestFixedVF); VF *= 2) {
----------------
nit: Is it worth doing:

  for (bool Predicated : { false, true }) {
    for (ElementCount VF = ElementCount::getFixed(2), ...)
      AddVariantDecl(VF, Predicated)
    for (ElementCount VF = ElementCount::getScalable(2), ...)
      AddVariantDecl(VF, Predicated)
  }


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:645
          ElementCount::isKnownLE(VF, WidestFixedVF); VF *= 2)
-      Scalarize &= !TLI.isFunctionVectorizable(ScalarName, VF);
+      Scalarize &= !TLI.isFunctionVectorizable(ScalarName, VF, false);
     for (ElementCount VF = ElementCount::getScalable(1);
----------------
I feel a bit uncomfortable about this code, because there may be two variants available, a masked and an unmasked variant, where one may be vectorizable and the other one is not. I wonder if `isFunctionVectorizable` should return `true` when `false` is passed there is a predicated variant available. That should be safe, because it is always valid to use a predicated vector function when the vector loop itself doesn't require predication (although the opposite doesn't hold). It would require changing the interface a bit to also return whether the vectorizable function is predicated.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll:16
+  ; NEON:     [[TMP5:%.*]] = call <2 x double> @_ZGVnN2v_acos(<2 x double> [[TMP4:%.*]])
+  ; SVE:      [[TMP5:%.*]] = call <vscale x 2 x double> @_ZGVsMxv_acos(<vscale x 2 x double> [[TMP4:%.*]])
   ; CHECK:    ret void
----------------
Given that these functions are described as masked:

  TLI_DEFINE_VECFUNC("acos", "_ZGVsMxv_acos",  SCALABLE(2), MASKED)
  TLI_DEFINE_VECFUNC("acosf", "_ZGVsMxv_acosf", SCALABLE(4), MASKED)

Should these calls not pass a predicate mask operand?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146839