[PATCH] D106678: [PowerPC] Add pwr7 and pwr10 support to IBM MASSV pass on AIX

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour accepted this revision.
bmahjour added a comment.
This revision is now accepted and ready to land.

LGTM with a request to add a TODO comment.



================
Comment at: llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp:77
+    return "";
+  if (Subtarget->isAIXABI() && Subtarget->hasP10Vector())
+    return "_P10";
----------------
masoud.ataei wrote:
> bmahjour wrote:
> > 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.
> 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?  
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).


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-altivec.ll:16
 ; CHECK-LABEL: @cbrt_f64(
-; CHECK-NOT: __cbrtd2_P8{{.*}}<2 x double>
 ; CHECK: ret void
----------------
masoud.ataei wrote:
> bmahjour wrote:
> > 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)?
> 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. 
Ok, thanks for checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106678