[TLI][AArch64] Extend SLEEF vectorized functions mapping with VLA functions

[pawosm01]

Summary

This commit extends D134719 "[AArch64] Enable libm vectorized
functions via SLEEF" with the mappings for the scalable functions.

It also introduces all the necessary changes needed to support masked
interfaces.

[pawosm01]

formatting update only

[pawosm01]

one more formatting change

[sdesmalen]

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 :)

llvm/lib/Transforms/Utils/InjectTLIMappings.cpp

43

nit: Is there a reason you've named this GlobalPredicate instead of just Predicate ?

113–119

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)
}

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

645 ↗

(On Diff #508498)

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.

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

16

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?

[danielkiss]

I'd add note to the documentation too when scalable vectors are used the sleef library is expect to be compiled with it.

llvm/include/llvm/Analysis/VecFuncs.def

21–23

I won't introduce a new argument here, since MASKED is just indicate this interface is scalable so far.
Can we make the declarations less verbose?
e.g.:
#define FIXED(NL) ElementCount::getFixed(NL), false
#define SCALABLE(NL) ElementCount::getScalable(NL), true

llvm/lib/Analysis/VectorUtils.cpp

1532

for me the "Global" naming is a bit misleading or am I missing something?
Masked maybe?

[pawosm01]

post-review changes

[pawosm01]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

645 ↗

(On Diff #508498)

Considering this is the only place where this overload is used, there is no point in changing or extending the interface. I've reverted to old parameter list and handle your idea inside of the overload of isFunctionVectorizable() called from here.

[pawosm01]

formatting changes again

[sdesmalen]

llvm/include/llvm/Analysis/VecFuncs.def

21–23

Sorry I missed this comment yesterday, but I'm not sure I agree with the suggestion to let SCALABLE imply MASKED. The two concepts are orthogonal, they just happen to have the same value for the set of vector function mappings added in this patch. It's perfectly possible to have an unmasked scalable interface and we may want to add those in the future.

The cost of adding the mask separately seemed quite low, is there another reason you prefer not to add this now?

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

159 ↗

(On Diff #508793)

Can you make false a default option for this argument, so that you don't explicitly have to pass it?

[pawosm01]

Restored to the previous idea. And added default param value as suggested.

[pawosm01]

llvm/include/llvm/Analysis/VecFuncs.def

21–23

I tend to agree with @sdesmalen so I'm reverting to the previous shape.

[pawosm01]

llvm/lib/CodeGen/ReplaceWithVeclib.cpp

159 ↗

(On Diff #508793)

Yes

[pawosm01]

removed extra spaces that suddenly appeared in two lines.

[sdesmalen]

llvm/include/llvm/Analysis/VecFuncs.def

702

Does this need:

#undef MASKED
#undef NOMASK

?

llvm/lib/Analysis/TargetLibraryInfo.cpp

1184

When you #undef MASK/NOMASK in the .def file, this needs changing to something like:

/*MASK=*/false

[pawosm01]

post-review changes again

[pawosm01]

llvm/include/llvm/Analysis/VecFuncs.def

702

It does need it indeed.

[sdesmalen]

LGTM, thanks for addressing all comments.

llvm/test/Transforms/LoopVectorize/AArch64/sleef-calls-aarch64.ll

175

When enabling tail-folding (enabled with -sve-tail-folding=all) the loop no longer vectorizes because there is an Invalid cost for the call to atan2. This is unexpected, because intrinsic is predicated and so could handle the tail-folded loop.

Nothing that needs fixing in this patch, I'm just pointing out that there is more work to follow-on from this patch to benefit from the masked vector-function variants.

[danielkiss]

LGTM, thanks!

llvm/include/llvm/Analysis/VecFuncs.def

21–23

I have no objection since the interface is the struct VecDesc, here the macro would just save some repetition.