[PowerPC] Add target feature requirement to builtins

[qiucf]

Summary

Clang has mechanism to specify required target features of a built-in function. This patch adds such definitions to Altivec, VSX, HTM, PairedVec and MMA builtins.

This will help frontend to detect incompatible target features of bulitin when using __attribute__((target("feature"))) syntax. For example,

__attribute__((target("no-vsx")))
void foo(vector double* d) {
  vector double a, b;
  *d = __builtin_vsx_xvmaxdp(a, b); // This would crash without this patch.
}

[qiucf]

Ping

[shchenz]

I think this is a good direction. For example we can avoid the crash in https://github.com/llvm/llvm-project/issues/60959 and give a clear diagnostic message.

However, manually adding the required target feature seems a little mistakable, like the one below. I guess we can not get the required feature in the LLVM instruction TDs(if the builtin is mapped to a IR intrinsic and the intrinsic is selected inside the instruction TD) because this is done in CLANG?
Really need to way to make sure that the instruction are marked with the accurate target feature, if not possible to do this inside the compiler, an offline script maybe? Checking the instruction one by one is a little time-consuming.

clang/include/clang/Basic/BuiltinsPPC.def

524

These builtins vabsdub, vabsduh, vabsduw should require ISA3.0 which is not altivec or vsx. Do we have a reasonable feature for Power9 instructions, power9-vector maybe?

[qiucf]

However, manually adding the required target feature seems a little mistakable, like the one below. I guess we can not get the required feature in the LLVM instruction TDs(if the builtin is mapped to a IR intrinsic and the intrinsic is selected inside the instruction TD) because this is done in CLANG?

Yes, this is how clang builtin definitions work. They are independent from LLVM definitions in tablegen files.

Really need to way to make sure that the instruction are marked with the accurate target feature, if not possible to do this inside the compiler, an offline script maybe? Checking the instruction one by one is a little time-consuming.

I actually don't think we need extra script to maintain this. They're rather static (not willing to change after written), independent (one changed won't affect another) and human-readable.

clang/include/clang/Basic/BuiltinsPPC.def

524

Thanks. I'll update then. power9-vector is good option. But I'm curious what's the different pratical usages from power9-vector and isa-v30-instructions. Maybe cc @nemanjai

[nemanjai]

clang/include/clang/Basic/BuiltinsPPC.def

524

We don't want ISA 3.0 to imply that it is OK to use vector instructions. For example, most of the distros are now Power9 and up. So the kernel is built with -mcpu=power9. However, the kernel is also built with -mno-altivec so we don't want to allow any use of vector instructions/registers in the kernel while we want ISA 3.0 instructions.
And that's just one use case. Ultimately, we keep these separate so we can control scalar ISA<N> instructions independently of the vector ones.

[nemanjai]

Another note regarding the motivating test case:
Use of vector double should require VSX. We don't really seem to have the ability to turn this off early enough to catch this though. It would seem that in the front end, the target features depend on the compilation options and not the function itself.

As a result, the ABI of the following:

__attribute__((target("no-vsx"))) vector double foo(vector double *d) {
  return *d + (vector double) { 4.5, 34.9 };
}

is different than it would be if the attribute wasn't there.

[qiucf]

In D143467#4182457, @nemanjai wrote:

Another note regarding the motivating test case:
Use of vector double should require VSX. We don't really seem to have the ability to turn this off early enough to catch this though. It would seem that in the front end, the target features depend on the compilation options and not the function itself.

Thanks for reminding me of vector double exception!

I have D143479 to make target feature check respect target attributes. I will have a look about __attribute__((target("feature"))) case.

[qiucf]

• For builtins with matching instruction, use the required ISA/vector version
• For the rest builtins used in altivec.h, use requirements specified by the header
• Keep the feature checks in SemaChecking as-is, since they give user useful message (only available on POWER8 or later CPUs instead of requires isa-v207-instructions to be enabled)
  • lharx and similar instructions exist since ISA v2.06 (Power 7), while SemaChecking.cpp requires ISA v2.07 (Power 8)

[nemanjai]

In D143467#4195474, @qiucf wrote:

• For builtins with matching instruction, use the required ISA/vector version
• For the rest builtins used in altivec.h, use requirements specified by the header
• Keep the feature checks in SemaChecking as-is, since they give user useful message (only available on POWER8 or later CPUs instead of requires isa-v207-instructions to be enabled)
  • lharx and similar instructions exist since ISA v2.06 (Power 7), while SemaChecking.cpp requires ISA v2.07 (Power 8)

There are quite a number of Power7 CPU's that do not have l[bh]arx. These were a late addition (i.e. in 2.06b). We should only emit it on Power8.

[nemanjai]

Actually, while you're here, can you please remove all calls to SemaFeatureCheck() that test builtins which are only available on a specific CPU? The Sema checking happens too early before the target info is populated, which does not allow for a very common idiom where the feature is enabled for a function/region using an attribute or a pragma.

For example, we received a report from Eigen that something like this:

__attribute__((target("mma,prefix-instrs")))
void test5(unsigned char *vqp, unsigned char *vpp, vector unsigned char vc,
           unsigned char *resp) {
  __vector_quad vq = *((__vector_quad *)vqp);
  __vector_pair vp = *((__vector_pair *)vpp);
  __builtin_mma_pmxvf64ger(&vq, vp, vc, 0, 0);
  *((__vector_quad *)resp) = vq;
}

vector unsigned char test6(vector unsigned char a, vector unsigned char b) {
  return a + b + (vector unsigned char)121;
}

Cannot be done with Clang because it produces errors such as:

error: this builtin is only valid on POWER10 or later CPUs
  __builtin_mma_pmxvf64ger(&vq, vp, vc, 0, 0);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

clang/include/clang/Basic/BuiltinsPPC.def

83–85

Please change this to 207.

87–89

Similarly to the loads.

[qiucf]

• Require ISA v2.07 for __builtin_ppc_l[hb]arx __builtin_ppc_st[hb]cx.
• Remove sema checks for target features.
• Refactor a lot of cases since the target feature check happens in codegen phase.

[nemanjai]

This looks fine to me. I'd like some of the devs that have added builtins with Sema checking in the past to have a look here as well. @amyk @maryammo @kamaub @stefanp Can you have a look at this as well please?

[stefanp]

Overall I think that this looks fine to me as well.
I had a couple of minor comments and you may decide that you don't need to do either one so if that's the case just mention why in a comment and I will approve the patch.

clang/include/clang/Basic/BuiltinsPPC.def

987

Based on the original implementation in SemaBuiltinPPCMMACall all of the mma builtins also require paired-vector-memops.
Is this something that we still need?

clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-test.c

47

nit:
Should this be

... needs target feature vsx

Instead of listing them both?

Fixing this might be more trouble than it's worth because you would have to edit CodeGenFunction::checkTargetFeatures. I just thought I would mention it.

[amyk]

Overall looks OK to me, as well. I just had two questions that I wanted to ask.

clang/include/clang/Basic/BuiltinsPPC.def

444

Does this need to be vsx?

820

Should this be power10-vector?

[maryammo]

It looks good to me, just added a minor question as I was not able to verify that.

clang/include/clang/Basic/BuiltinsPPC.def

444

How do we find the appropriate FEATURE for the above 4 builtins? (first 3 are p9 and the 4th one is altivec)