This is an archive of the discontinued LLVM Phabricator instance.

[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 Timeline

qiucf created this revision.Feb 6 2023, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 11:29 PM
Herald added a subscriber: kbarton. · View Herald Transcript
qiucf requested review of this revision.Feb 6 2023, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 6 2023, 11:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 added a comment.Mar 9 2023, 1:26 AM

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 added inline comments.Mar 9 2023, 12:05 PM
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 added a comment.EditedMar 9 2023, 12:12 PM

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 added a comment.EditedMar 13 2023, 10:40 PM

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 updated this revision to Diff 505370.Mar 14 2023, 10:59 PM
qiucf marked 2 inline comments as done.
  • 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)
  • 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.

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 updated this revision to Diff 508528.Mar 27 2023, 1:47 AM
qiucf marked 2 inline comments as done.
  • 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 accepted this revision.Mar 31 2023, 6:35 AM
nemanjai added subscribers: maryammo, kamaub, stefanp.

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?

This revision is now accepted and ready to land.Mar 31 2023, 6:35 AM

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–72

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 added a comment.Mar 31 2023, 11:46 AM

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?

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)

kamaub added a comment.Apr 3 2023, 3:01 PM

Can you add a PowerPC codegen test case for __attribute__((target(? All of the updated test cases seem to only test -target-feature.
The only test case we have for __attribute((target( is a sema test ./clang/test/Sema/ppc-attr-target-inline.c.

Converting the deleted clang/test/Sema/ppc-mma-builtins.c and clang/test/Sema/ppc-paired-vector-builtins.c to a codegen test cases
like clang/test/CodeGen/PowerPC/builtins-ppc-htm.c using FileCheck seems like a nice solution since it would reintroduce the testing
for +paired-vector-memops,-mma situations, as well as a for __attribute__((target("no-mma")))

clang/include/clang/Basic/BuiltinsPPC.def
987

since we are able to supply a comma separated list as done with TARGET_BUILTIN(__builtin_ppc_compare_exp_uo, "idd", "", "isa-v30-instructions,vsx") @ clang/include/clang/Basic/BuiltinsPPC.def:105we should definitely also specify paired-vector-memops,mma for the [UNALIASED_]CUSTOM_BUILTINs previously covered under the default case of SemaBuiltinPPCMMACall()

qiucf updated this revision to Diff 511260.Apr 5 2023, 8:06 PM
qiucf marked 2 inline comments as done.
qiucf added inline comments.
clang/include/clang/Basic/BuiltinsPPC.def
444

vcmpneb vcmpneh vcmpnew debute in ISA 3.0. vcmpned does not exist, so it's keeped as-is. (but vector long long requires vsx, so it's reasonable to require vsx)

987

Since mma and paired-vector-memops are independent from each other, I think only assemble/disassemble builtins should require paired-vector-memops?

clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-test.c
47–72

Yes. That can be done in another patch.

qiucf added inline comments.Apr 6 2023, 1:35 AM
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-test.c
47–72

After checking the source code, I found it might be too complex to filter existing features out. Because it actually has a syntax to support 'and'/'or' of feature requirements: https://github.com/llvm/llvm-project/blob/4016e5d/clang/lib/Basic/BuiltinTargetFeatures.h#L22_L31

qiucf updated this revision to Diff 512342.Apr 10 2023, 10:38 PM

Require paired-vector-memops for MMA operations.

kamaub requested changes to this revision.Apr 11 2023, 8:59 AM

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.

Can you add a PowerPC codegen test case for __attribute__((target(? All of the updated test cases seem to only test -target-feature.
The only test case we have for __attribute((target( is a sema test ./clang/test/Sema/ppc-attr-target-inline.c.

Converting the deleted clang/test/Sema/ppc-mma-builtins.c and clang/test/Sema/ppc-paired-vector-builtins.c to a codegen test cases
like clang/test/CodeGen/PowerPC/builtins-ppc-htm.c using FileCheck seems like a nice solution since it would reintroduce the testing
for +paired-vector-memops,-mma situations, as well as a for __attribute__((target("no-mma")))

This revision now requires changes to proceed.Apr 11 2023, 8:59 AM
qiucf updated this revision to Diff 512685.Apr 12 2023, 1:26 AM
qiucf added a comment.Apr 12 2023, 1:29 AM

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.

Can you add a PowerPC codegen test case for __attribute__((target(? All of the updated test cases seem to only test -target-feature.
The only test case we have for __attribute((target( is a sema test ./clang/test/Sema/ppc-attr-target-inline.c.

Converting the deleted clang/test/Sema/ppc-mma-builtins.c and clang/test/Sema/ppc-paired-vector-builtins.c to a codegen test cases
like clang/test/CodeGen/PowerPC/builtins-ppc-htm.c using FileCheck seems like a nice solution since it would reintroduce the testing
for +paired-vector-memops,-mma situations, as well as a for __attribute__((target("no-mma")))

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.

qiucf added a comment.Apr 23 2023, 7:49 PM

Gentle ping .. @kamaub

kamaub accepted this revision.Apr 27 2023, 12:41 PM

Hello, sorry for missing you ping and delaying the patch so long just for test case adjustments, thank you for addressing them.
Everything LGTM but lei and I had one request that can be made before you commit:

Please split the two functions and associated run line from ppc-p10-feature-builtins.c into two files named as follows
ppc-p10-mma-builtin-err.c
ppc-p10-paired-vec-memops-builtin-err.c

This revision is now accepted and ready to land.Apr 27 2023, 12:41 PM
qiucf updated this revision to Diff 520306.May 8 2023, 2:31 AM
This revision was landed with ongoing or failed builds.May 8 2023, 2:54 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.May 8 2023, 11:17 AM
This revision is now accepted and ready to land.May 8 2023, 11:17 AM
qiucf closed this revision.May 10 2023, 12:45 AM