This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][NFC] Expand coverage of ReplaceWithVeclib testing using SLEEF vector library
ClosedPublic

Authored by jolanta.jensen on Jul 18 2023, 10:12 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 10:12 AM
jolanta.jensen requested review of this revision.Jul 18 2023, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 10:12 AM
mgabka added inline comments.Jul 19 2023, 1:52 AM
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.

jolanta.jensen added inline comments.Jul 19 2023, 4:46 AM
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll
7

Corrected. Also added vscale to function names.
A comment about TLI mappings added.

mgabka added inline comments.Jul 20 2023, 12:34 AM
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.

jolanta.jensen added inline comments.Jul 20 2023, 3:00 AM
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef-scalable.ll
7

Added to the RUN line.

Matt added a subscriber: Matt.Jul 20 2023, 10:17 AM

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

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.

paulwalker-arm added a comment.EditedJul 21 2023, 2:54 AM

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

OK then I think we are still missing:

llrint
llround
lrint
nearbyint

as those are defined in libm.

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.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 21 2023, 6:46 AM
This revision was automatically updated to reflect the committed changes.
mgabka reopened this revision.Jul 21 2023, 6:53 AM

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.

jolanta.jensen retitled this revision from [AArch64][NFC] Expand SLEEF coverage for ReplaceWithVeclib testing to [AArch64][NFC] Expand coverage of ReplaceWithVeclib testing using SLEEF vector library.Jul 21 2023, 10:25 AM
jolanta.jensen edited the summary of this revision. (Show Details)
jolanta.jensen edited the summary of this revision. (Show Details)
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef.ll
187

Reordered so log10 is before log2.

mgabka accepted this revision.Jul 24 2023, 3:23 AM

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.

This revision is now accepted and ready to land.Jul 24 2023, 3:23 AM
This revision was landed with ongoing or failed builds.Jul 26 2023, 1:58 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/AArch64/replace-intrinsics-with-veclib-sleef.ll