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

vitalybuka added a comment. Breaks these bots? https://lab.llvm.org/buildbot/#/builders/18/builds/8898 https://lab.llvm.org/buildbot/#/builders/19/builds/16412 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143467/new/ https://reviews.llvm.org/D1434

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

kamaub accepted this revision. kamaub added a comment. This revision is now accepted and ready to land. 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 be

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

qiucf added a comment. Gentle ping .. @kamaub Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143467/new/ https://reviews.llvm.org/D143467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

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

qiucf added a comment. In D143467#4258380 , @kamaub wrote: > 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. > > In D143467#42416

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

kamaub requested changes to this revision. kamaub added a comment. This revision now requires changes to proceed. 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. In D143467#42

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

qiucf added inline comments. 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) -//

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

qiucf added inline comments. Comment at: clang/include/clang/Basic/BuiltinsPPC.def:444 +TARGET_BUILTIN(__builtin_altivec_vcmpnew_p, "iiV4iV4i", "", "power9-vector") +TARGET_BUILTIN(__builtin_altivec_vcmpned_p, "iiV2LLiV2LLi", "", "altivec") + maryammo wrote: > am

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

kamaub added a comment. 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 dele

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

maryammo added a comment. It looks good to me, just added a minor question as I was not able to verify that. Comment at: clang/include/clang/Basic/BuiltinsPPC.def:444 +TARGET_BUILTIN(__builtin_altivec_vcmpnew_p, "iiV4iV4i", "", "power9-vector") +TARGET_BUILTIN(__builtin_altive

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

amyk added a comment. Overall looks OK to me, as well. I just had two questions that I wanted to ask. Comment at: clang/include/clang/Basic/BuiltinsPPC.def:444 +TARGET_BUILTIN(__builtin_altivec_vcmpnew_p, "iiV4iV4i", "", "power9-vector") +TARGET_BUILTIN(__builtin_altivec_vcmpne

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

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/cl

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

nemanjai accepted this revision. nemanjai added subscribers: maryammo, kamaub, stefanp. nemanjai added a comment. This revision is now accepted and ready to land. 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.

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

nemanjai added a comment. 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 whe

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

nemanjai added a comment. 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 chec

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

qiucf added a comment. 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

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

nemanjai added a comment. 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 op

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

nemanjai added inline comments. Comment at: clang/include/clang/Basic/BuiltinsPPC.def:491 +TARGET_BUILTIN(__builtin_altivec_vabsduh, "V8UsV8UsV8Us", "", "altivec") +TARGET_BUILTIN(__builtin_altivec_vabsduw, "V4UiV4UiV4Ui", "", "altivec") qiucf wrote: > shchenz

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

qiucf added a comment. > 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 instruct

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

shchenz added a comment. 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

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

qiucf added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143467/new/ https://reviews.llvm.org/D143467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

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

qiucf created this revision. qiucf added reviewers: nemanjai, sfertile, amyk, shchenz, lkail, PowerPC. Herald added a subscriber: kbarton. Herald added a project: All. qiucf requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Clang has mechanis