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

[Masoud Ataei via Phabricator via llvm-commits]

masoud.ataei added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCLowerMASSVEntries.cpp:74
 StringRef PPCLowerMASSVEntries::getCPUSuffix(const PPCSubtarget *Subtarget) {
-  // Assume Power8 when Subtarget is unavailable.
+  // Assume generic when Subtarget is unavailable.
   if (!Subtarget)
----------------
bmahjour wrote:
> Do these generic entries actually exist in MASS (ie can link correctly)?
Yes, these entries exist in AIX and Linux MASS (xlopt library). So they will link correctly.


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


================
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
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106678