[PATCH] D143467: [PowerPC] Add target feature requirement to builtins

[Stefan Pintilie via Phabricator via cfe-commits]

stefanp added a comment.

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.



================
Comment at: clang/include/clang/Basic/BuiltinsPPC.def:987
+
+UNALIASED_CUSTOM_BUILTIN(mma_assemble_acc, "vW512*VVVV", false, "mma")
+UNALIASED_CUSTOM_BUILTIN(mma_disassemble_acc, "vv*W512*", false, "mma")
----------------
Based on the original implementation in `SemaBuiltinPPCMMACall` all of the `mma` builtins also require `paired-vector-memops`. 
Is this something that we still need?


================
Comment at: clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-test.c:47
 
-int test_test_data_class_f() {
-// CHECK-LABEL:       @test_test_data_class_f
-// CHECK:             [[TMP:%.*]] = call i32 @llvm.ppc.test.data.class.f32(float %0, i32 127)
-// CHECK-NEXT:        ret i32 [[TMP]]
-// CHECK-NONPWR9-ERR: error: this builtin is only valid on POWER9 or later CPUs
-// CHECK-NOVSX-ERR: error: this builtin requires VSX to be enabled
-  return __test_data_class(f, 127);
+// CHECK-NOVSX-ERR: error: '__builtin_ppc_compare_exp_uo' needs target feature isa-v30-instructions,vsx
+// CHECK-NOVSX-ERR: error: '__builtin_ppc_compare_exp_lt' needs target feature isa-v30-instructions,vsx
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143467