Paths
| Differential D143467
[PowerPC] Add target feature requirement to builtins ClosedPublic Authored by qiucf on Feb 6 2023, 11:29 PM.
Details 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.
}
Diff Detail
Event TimelineComment Actions 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?
Comment Actions
Yes, this is how clang builtin definitions work. They are independent from LLVM definitions in tablegen files.
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.
Comment Actions Another note regarding the motivating test case: 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. Comment Actions
Thanks for reminding me of vector double exception!
qiucf marked 2 inline comments as done. Comment Actions
Comment Actions
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. Comment Actions 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); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
qiucf marked 2 inline comments as done. Comment Actions
This revision is now accepted and ready to land.Mar 31 2023, 6:35 AM Comment Actions Overall I think that this looks fine to me as well.
Comment Actions It looks good to me, just added a minor question as I was not able to verify that.
Comment Actions Can you add a PowerPC codegen test case for __attribute__((target(? All of the updated test cases seem to only test -target-feature. Converting the deleted clang/test/Sema/ppc-mma-builtins.c and clang/test/Sema/ppc-paired-vector-builtins.c to a codegen test cases
qiucf marked 2 inline comments as done. qiucf added inline comments.
Comment Actions Sorry I should have requested changes before for this comment below, but I do want these test moved to codegen and expanded, please let me know if anything is unclear.
This revision now requires changes to proceed.Apr 11 2023, 8:59 AM Comment Actions
Hi, I updated the case to test builtins used in previous Sema tests when both no-mma and no-paired-vector-memops. There's limitation that such codegen error message only diagnose one function, so I wrapped them into a single function. Not sure if that's the full meaning. Comment Actions Hello, sorry for missing you ping and delaying the patch so long just for test case adjustments, thank you for addressing them. Please split the two functions and associated run line from ppc-p10-feature-builtins.c into two files named as follows This revision is now accepted and ready to land.Apr 27 2023, 12:41 PM This revision was landed with ongoing or failed builds.May 8 2023, 2:54 AM Closed by commit rG651b0e2e7afc: [PowerPC] Add target feature requirement to builtins (authored by qiucf). · Explain Why This revision was automatically updated to reflect the committed changes. vitalybuka added a reverting change: rGaf88d34f0552: Revert "[PowerPC] Add target feature requirement to builtins".May 8 2023, 11:17 AM This revision is now accepted and ready to land.May 8 2023, 11:17 AM
Revision Contents
Diff 520309 clang/include/clang/Basic/BuiltinsPPC.def
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Basic/Targets/PPC.cpp
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/Sema/SemaChecking.cpp
clang/test/CodeGen/PowerPC/builtins-ppc-altivec.c
clang/test/CodeGen/PowerPC/builtins-ppc-fma.c
clang/test/CodeGen/PowerPC/builtins-ppc-fpconstrained.c
clang/test/CodeGen/PowerPC/builtins-ppc-htm.c
clang/test/CodeGen/PowerPC/builtins-ppc-p7-disabled.c
clang/test/CodeGen/PowerPC/builtins-ppc-p8vector.c
clang/test/CodeGen/PowerPC/builtins-ppc-pwr10-64bit.c
clang/test/CodeGen/PowerPC/builtins-ppc-vsx.c
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-pwr8.c
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-pwr9-64bit.c
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-pwr9.c
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-test.c
clang/test/CodeGen/PowerPC/ppc-p10-mma-builtin-err.c
clang/test/CodeGen/PowerPC/ppc-p10-paired-vec-memops-builtin-err.c
clang/test/Sema/ppc-mma-builtins.c
clang/test/Sema/ppc-paired-vector-builtins.c
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please change this to 207.