This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add pwr7 and pwr10 support to IBM MASSV pass on AIX
ClosedPublic

Authored by masoud.ataei on Jul 23 2021, 9:17 AM.

Details

Summary

Before MASSV only supported P8 and P9 on AIX ans Linux . This patch proposes MASSV
to add support of P7 and P10 only on AIX too.

PS. On linux, MASS library doesn't support P7 and currently is under development for P10.

Diff Detail

Event Timeline

masoud.ataei created this revision.Jul 23 2021, 9:17 AM
masoud.ataei requested review of this revision.Jul 23 2021, 9:17 AM
bmahjour added inline comments.Jul 26 2021, 10:00 AM
llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp
74

Do these generic entries actually exist in MASS (ie can link correctly)?

77

why not just check for Subtarget->hasP10Vector()?
I understand P10 work on LoP is in progress, but not sure about the state of that work. This patch causes an observable change in behaviour where we used to resolve to P8 and link successfully on P10, but now we abort with the fatal error below. If P10 entries do currently exist on LoP P10 and they are functional (perhaps not with best performance), then we should just let it link with P10 entries. Otherwise we should add a comment and link to P8 entries like before.

llvm/test/Transforms/LoopVectorize/PowerPC/massv-altivec.ll
16

I'm wondering if it is possible to get these _P8 calls in the IR in any other way (ie does clang generate them to implement any of the builtins)?

masoud.ataei added inline comments.Jul 26 2021, 10:45 AM
llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp
74

Yes, these entries exist in AIX and Linux MASS (xlopt library). So they will link correctly.

77

Currently, on Linux or AIX, if user compile for P10, we will get the _P9 version (not _P8). This patch doesn't propose any changing behavior for Linux. On linux, if you have -mcpu=pwr10 you will still getting _P9 version (not an fatal error) even after this patch applied. (when target is P10, both hasP9Vector() and hasP8Vector() are returning true)

On linux MASS, there are still no entities with suffix _P10. It is possible to add them in MASS (which will be P9 optimized MASS functions but only with suffix _P10 for now) and remove the check for` isAIXABI()` in this pass. With above explanations, do you still think this is more desirable?

llvm/test/Transforms/LoopVectorize/PowerPC/massv-altivec.ll
16

I am not aware of any builtin calls which are resolving to MASS calls in compiler. And I think there is none. Clang may (or will) resolve some builtin to math calls but (it seems) not to MASS calls. I think MASS calls are only generated in MASS pass(es).

I tired to find any other place that clang/LLVM generate calls to _P8 (or any other MASS functions) but I couldn't find.

bmahjour accepted this revision.Jul 26 2021, 3:10 PM

LGTM with a request to add a TODO comment.

llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp
77

Ultimately the check would have to be changed to check for P10 only (regardless of the OS), but we can do that later and my main concern was about change of behaviour. I guess there would still be a difference in behaviour for someone compiling with -mcpu=pwr10 -Xclang -target-feature -Xclang -power9-vector but probably nobody is doing that.
I don't have a strong preference for adding P10 entries that are P9 optimized, so I think we can leave this the way it is then (except maybe add a todo comment).

llvm/test/Transforms/LoopVectorize/PowerPC/massv-altivec.ll
16

Ok, thanks for checking.

This revision is now accepted and ready to land.Jul 26 2021, 3:10 PM
masoud.ataei closed this revision.Jul 26 2021, 4:27 PM
masoud.ataei marked 5 inline comments as done.