This patch expands testing coverage for ReplaceWithVeclib pass
when SLEEF vector library is used. It adds testing for all LLVM
intrinsics which correspond to math functions from libm.
llrint, llround and lrint are not included as currently
IR verifier pass does not allow to use vector types with them.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll | ||
---|---|---|
7 | the tests in this file are incorrect, the signatures and declarations used for llvm intrinsics are using fixed width type, not a scalable. A comment explaining why the mappings are not used (despite existing in the TLI) would be also useful. |
Corrected the signatures and declarations for llvm intrinsics in tests using scalable vectors.
Added a comment why existing TLI mappings for scalable vectors are not used.
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll | ||
---|---|---|
7 | Corrected. Also added vscale to function names. |
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll | ||
---|---|---|
7 | I think you are also missing sve attribute either in the RUN line or added to functions, both are acceptable solutions to me. |
Added sve attribute to the RUN line in replace-intrinsics-with-veclib-sleef-scalable.ll test.
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll | ||
---|---|---|
7 | Added to the RUN line. |
If the intention of this patch is to add tests for all LLVM math intrinsics (as you stated in "This patch expands SLEEF coverage for ReplaceWithVeclib testing
to all supported math intrinsics."), then I think you do not have here full coverage, for example I do not see tests for llvm.abs, llvm.smax, llvm.smin, llvm.powi, and several other intrinsics
full list of intrinsics is available here https://llvm.org/docs/LangRef.html#standard-c-c-library-intrinsics
Well, it reads "all supported math intrinsics" The ones you mention are not supported by SLEEF.
It's not so much about SLEEF but math.h. The intention of this patch is full coverage of the intrinsics that are used to replace calls to math.h functions. This is why the integer related intrinsics have been omitted. The above link was used with intrinsics filtered based on those that specifically referenced libm.
OK then I think we are still missing:
llrint
llround
lrint
nearbyint
as those are defined in libm.
Also in my opinion the commit message should directly reference libm not use "math intrinsics" as abs is also a math operation, but it is not defined in libm
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef.ll | ||
---|---|---|
187 | nit: I think that log10 should be before log2 if we are aiming to keep alphabetical order |
ah I see there reason why they are not included, https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Verifier.cpp#L5780, worth to mention in directly I think.
sorry, I messed up, and accidentally closed this revision, but the patch wasn't merged.
I am reopening it.
Reworked commit message to be more informative.
Reordered tests so log10 comes before log2.
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef.ll | ||
---|---|---|
187 | Reordered so log10 is before log2. |
Thanks for updating the patch, it LGTM.
I realised one more thing, the "fast" flag attached to the calls in the tests is not needed.
The replace-with-veclib pass does not check for it and and performs the transformation always when the mappings exist.
However in this pass there is a chunk of code ensuring that the fast math flags are preserved, so I think we can keep it in your tests as it is.
the tests in this file are incorrect, the signatures and declarations used for llvm intrinsics are using fixed width type, not a scalable.
A comment explaining why the mappings are not used (despite existing in the TLI) would be also useful.