LGTM. Thanks for reviving this patch.

In D152914#4602914, @ilinpv wrote:

Friendly ping, are there any questions remained to proceed with target-independent __builtin_cpu_supports ?

LGTM. The patch is easy to follow now. Thank you for your patience.

The reason 64 seems to be a good value on PPC chips is hopefully a transient situation that seems to have something to do with how bad the chips can be with branch prediction for indirect branches (CTR branches). I think at least some indication should be added in code comments suggesting this be re-evaluated on future HW that will hopefully do better with this kind of branch.
The other knob we have access to wrt. jump tables (density) could probably use some tuning as well.

LGTM. This is a good idea and we should go ahead with this for anyone that uses vec_promote, but it might be a good idea to improve codegen for the insert which might be more common.

In D133103#4512023, @arsenm wrote:

Kill flags should be removed. You should switch to reverse liveness which does not rely on accurate kill flags

I'm not sure I follow. Are you suggesting that we remove all kill flags in this pass? We're really only fixing them up to appease the verifier rather than because anything in the PPC back end relies on the flags.

Thanks for implementing this.

In D158066#4593961, @qiucf wrote:

CC @amyk @quinnp Any comments about the naming?

I see some __builtin_ppc_xxx are aliased into __builtin_xxx by defineXLCompatMacros. But these are not XL-compatible builtins, and the macros do not always work.

This patch fixes a regression caused by a patch that is in v17. We will need to decide if we should backport this fix or if we should pull the original patch (which also has additional patches dependent on it that are in v17 as well). I think this patch is obvious enough to be backported, but I'll defer the final decision on that to @MaskRay once this is approved.
@amyk Perhaps it would help to perform a thorough test with the approved version of this patch applied to v17 to ensure it is safe to backport.

It seems to me that this section of this function has built up a significant amount of technical debt and we should really try to get it refactored so it is clearer what is happening and why. Please note that this is a statement about the existing implementation, not just about the updates in this patch.

Sorry I missed this. LGTM.

I think the code could be clarified a bit with some additional comments, but otherwise LGTM.

LGTM. Thanks.

It is not clear how someone in the future should react if the test case changes behaviour. You should think about maintainability of the test cases. It would appear that a part of the checks were produced using a script and a part was added manually. As such, it is not clear what the essential parts of the tests are so anyone maintaining this in the future must really dive deep into the TLS implementation to understand if changes are safe or not. I'm not sure what the best approach would be. Perhaps add a comment describing what is being tested. Or maybe only test the key parts using manual checks.

Seems fine to me - certainly from a PPC perspective.

I don't think the last set of comments were addressed yet. Requesting changes to remove it from my queue.

The patch certainly seems reasonable to me. The test case seems like it could be trimmed down a bit (I don't think all the DWARF directives are needed).

Thank you for the patch. This is actually interesting and is a bit of a change to how we thought the code was do be structured. For us the affinity between CodeGen and MCTargetDesc was an indication that the libraries, although separate, are dependent on each other. Of course, this is not necessary so I am in favour of this refactoring.

Ah, ok. This makes sense. When we don't know anything about the ABI, it makes sense to rely on -mabi.

In D156204#4530957, @qiucf wrote:

The motivation is https://github.com/llvm/llvm-project/issues/64080

I see both GCC and XLC 16 recognize the instruction in inlineasm.

The instruction (tlbilx) can be found in ISA 2.07 but eliminated in 3.0 and 3.1 since it belongs to 'Book III-E'. Not sure if it should be implemented in current PPC backend. So I put this in draft mode.

In support of the OpenPOWER Foundation’s standardization of server architecture, Power ISA Version 3.0 streamlined this integration by choosing a single Book III and a set of widely used categories to become part of the base architecture for all forward-looking Power implementations. All other optional architecture categories were eliminated to ensure increased application portability between Power processors. Legacy embedded applications that require the eliminated material will continue to use V. 2.07B.

-Preface of ISA 3.0

What version of the PowerISA is this from? What is the motivation for implementing this?

Sorry, I forgot to submit these comments when I reviewed this.

So we don't have code generation for this, but we are enabling it in the front end? What happens if we try to produce code for the IR this produces? It would make more sense to me to implement what is needed in the back end prior to allowing the front end to produce the code.

LGTM.

Please provide a description justifying this change. There is no context here for the viewer to determine whether this change makes sense.

LGTM.

@arsenm Has my description adequately addressed your question? Do you think we should move forward with [some version of] this patch or do you have any fundamental objections?

In D152914#4428692, @ilinpv wrote:

Thank you for the patch, it comes in the right time - we are also working on AArch64 __builtin_cpu_supports, and I was thinking how to make it more general.
I uploaded our RFC version for review https://reviews.llvm.org/D153153

It would be great to have in __builtin_cpu_supports argument string of plus-separated features. Just SemaBuiltinCpuSupports need to handle this.

Lets get this committed to unblock the bots and if it is not the correct/desired fix, the author can subsequently provide the more appropriate fix.

Thanks for adding this. Nice to common-up the behaviour. LGTM.

LGTM. Whatever the choice is wrt. 32-bit mode, please just add a comment to the function explaining it.

No concerns from the perspective of PowerPC here. Of course, my focus is primarily on the server side of things but I am not aware of any other group that could be adversely affected.

What is the plan for handling nearest, away rounding mode for which the PPC FPSCR does not have a setting?

In D138883#4479380, @tingwang wrote:

Rebase and added some comments.

Hi @nemanjai, I accepted and addressed your previous comments. Do you have any more concerns on the approach that is implemented here? Thank you!

LGTM.

Address comments

• Run clang-format
• Resolve issue with shift-extend peephole

It appears that this causes failures on buildbots: https://lab.llvm.org/buildbot/#/builders/230/builds/15327
Please fix or pull this.

In D154447#4472054, @stefanp wrote:

Unfortunately, this isn't just a case of adding:

def : Pat<(v2i64 (shl v2i64:$vA, (v2i64 (immEQOneV)))),
          (v2i64 (VADDUDM $vA, $vA))>;

like the others but I think it may be worth doing.

Yeah, this is because we don't have a way of materializing the <1, 1> vector so we end up with a constant pool load. We can provide custom legalization:
setOperationAction(ISD::SHL, MVT::v2i64, Custom); for Power8 and up where we would just leave the node alone if it's a shift by 1.

At this point maybe just add the test case and we can deal with the issue at a later date.

I agree this can be done in a follow-up patch.

LGTM aside from some minor nits.

LGTM.

LGTM.

In D148697#4422642, @PiotrZSL wrote:

In D148697#4422629, @nemanjai wrote:

It was also causing PPC bots to break. I fixed it in https://reviews.llvm.org/rGffd7a200fdfbd01ef296101647d2f2da91ddfd41

O, thank you. Thats explain a lot. Because when I look into cmake, clangTidyPerformanceModule were already there because you already added it.

In D148697#4421739, @Dinistro wrote:

In D148697#4421643, @PiotrZSL wrote:

@Dinistro It compiles & links with clang 16 also (shared libs). So I'm unable to reproduce.

I used the following command to build:

cmake -S/home/christianu/repos/llvm-project/llvm -B/home/christianu/repos/llvm-project/build-clang-tidy -GNinja -DLLVM_ENABLE_LLD=ON -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_ASSERTIONS=ON -DBUILD_SHARED_LIBS=ON -DLLVM_INCLUDE_UTILS=ON -DLLVM_INSTALL_UTILS=ON -DLLVM_BUILD_EXAMPLES=OFF -DLLVM_CCACHE_BUILD=ON -DLLVM_OPTIMIZED_TABLEGEN=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_ENABLE_RUNTIMES=openmp

LGTM. Thanks for fixing these.

Please note that this assert can trip even when disassembling with --disassemble and not just -disassemble-all. Namely, the offending word can appear in the text section (for example, the AIX traceback table, etc.).
This is very similar to https://reviews.llvm.org/rGc86f8d4276ae

Rebase on top of some new additions to the pass and address numerous review comments.

LGTM. Thanks for the fix.

Thank you so much for implementing this for PowerPC. Implementing JITLink has been on our radar for a while and this gives us a very welcome head start.

This all LGTM and I think the naming is consistent with other targets, so ppc64 makes sense. I have some minor comments that are more likely due to my lack of familiarity with jit-link than anything that should require action on the patch.

LGTM.

LGTM. Maybe give others a bit of time to chime in.

For the most part, the code produced for vector-promotion.ll is worse as we're forcing the operations into the vector space rather than doing it in scalar registers. Perhaps we should return false from this query if the input vector is the result of a load and the only user is the extract instruction. This would of course require that we change the interface of the query to take the extract instruction rather than the type.
The idea is that if we are loading the vector, extracting an element, performing an operation and then storing, we won't really load the vector but will load only the single element. Promoting the operation will force us to load the vector which is counter productive.

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?

My comments are minor nits that don't require another review, so LGTM.

This LGTM. Thanks for cleaning it up. Give it at least a week for any others that may have an opinion on this to chime in.