In D91050#2917401, @efriedma wrote:

In D91050#2915306, @steven.zhang wrote:

In D91050#2905380, @efriedma wrote:

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Passing a constant in，which has the alignment of the constant，return a constantpool，which need to specify the alignment of constantpool. We cannot query the alignment of constantpool now as the result could be cp + offset.

I think you're missing my point. If the caller has a particular idea of what alignment it needs, it will request it. The result will have at least that alignment. If the actual alignment comes out higher, it doesn't really matter.

In D91053#2913869, @shchenz wrote:

In D91053#2912214, @qiucf wrote:

In D91053#2912191, @lkail wrote:

A few months ago, steven shared his patch to us. For me, it should be an optimization of TOC layout, it looks odd to me to get it done at isel phase. I think we should figure out a new approach for this. What do you think @nemanjai @qiucf ?

I tend to not pursue this patch and might have a new one to get TOC layout optimization done.

I think it's located at DAG because we lower such constant pools stuff there. If this can be target independent (as Eli commented in previous patch) in DAG, or put it after ISel (I guess you mean that?), it's fine to abandon that.

I don't think after ISEL is a good idea. After ISel, the constant pool and the code pattern to access the constant pool are both settled down. It does not make sense to me to revert or change them later. It is better to generate it like we expect in the first place.

+1

In D91050#2905380, @efriedma wrote:

Passing an alignment in and returning an alignment out at the same time seems unlikely to be useful. DAGCombine can refine the alignment later anyway, if it turns out to be relevant. Maybe rename the version that returns the alignment out?

Also, if the point is literally just "glue a bunch of ConstantPools together", can we teach target-independent code to do that? It doesn't actually require anything target-specific, I think, and it should be useful on any target where computing the address of a constant pool isn't cheap.

In D106431#2907688, @whisperity wrote:

In D106431#2907002, @Sockke wrote:

Any thoughts? : )

First, let's first fix that we should still warn for the uninitialised enum case, without a FixIt. That's the issue at hand, right now, Clang-Tidy generates, as you identified, broken output. We can discuss the later steps after this is fixed. Please implement this logic, and update the patch, so we have a snapshot of how that would look like and the thing working.

+1

In D106431#2892866, @MTC wrote:

In D106431#2892859, @whisperity wrote:

Is this the right decision to make, conceptually? It will leave the variable uninitialised still, and reading such an uninit variable is still an issue, even if it is an enum.

Yeah, that's right. However, it's much more difficult to give enum an initial value than an integer.

Could we consider the alternative of warning the user about the uninitialized variable, just not offering an automatic (and potentially bad / incorrect) fix?

Make sense, we (ByteDance) are also hesitating whether we should provide automatic repair for uninitialized variables, because automatic fix may change the program semantics.

In D100162#2682340, @djtodoro wrote:

Does this solve the problem?

LGTM. But please do a complete test before enabling it by default.

Hi, @djtodoro, do you have any concern on this ? If not, I will land this patch days later.

Sorry, I don't mean to accept as PowerPC now ...

LGTM. But please hold on for some days to see if @nemanjai has more comments.

LGTM now. But please hold on for at least one week to see if @nemanjai has concern.

It seems that, craig is working on this bugzallia issue. Thank you for reporting this.

The bif/bic difference seems logically fine and probably better overall since we're using less registers, but someone who knows AArch should comment.
Also, it would be interesting to know why the code changed because we are seemingly producing the same set of SDNode ops? There might be another combine/lowering opportunity.

Split the BR_CC into another patch which will enhance the legalizer to support it. For SETCC/SELECT_CC, we will do it inside PowerPC as what other target did.

Address comments.

LGTM.

In D92083#2504734, @nemanjai wrote:

The description isn't really adequate - it does not answer my question at all really. The question is why not set the legalization action for BR_CC as Expand? Namely, what does the legalizer do that we don't want it to do if we let it expand the node? You just answered the question with the equivalent of "The legalizer does its job and that's not what we want." So please explain why that is not what we want.

Gentle ping...

@nemanjai Do you have any more comments on this ? Thank you!

Ping...

Thank you for doing this. LGTM now.

This is the same as what I want to do in D86681. As D86684 is still pending, it makes sense to split from it now.

LGTM. Please hold on for days to see if @nemanjai has more comments.

Rebase the patch.

Ping ...

LGTM, but please hold on for days to see if @craig.topper or @nemanjai have more comments.

It surprises to me that you didn't see the difference between LE and AIX, which doesn't make sense to me. Please double confirm to see if there is any conflict when update the test with the script.

Gentle ping ...

Hmm, so we can try to remove those parts that not relative with the parameter arsenm mentioned. I guess most of the check in the DAGCombiner could be removed if I understand correctly. To remove it completely, some work is needed for parameters or others that current IR cannot represent the semantics of the global setting. Is it right ?

• Reason why we need to change the interface

Ping...

Some code style comments.

Also LGTM.

In D93891#2473825, @spatel wrote:

We're trying to (very slowly...) move away from needing the global/target options. Also, I'm not sure how those translate with GlobalISel.
If there's a real-world case where the target options do not match the fast-math-flags on the IR, we should investigate if that was unexpected. Otherwise, I would prefer that we not add to the uses of target options.

In D92815#2472760, @nemanjai wrote:

I understand that the back end only needs Altivec to support the operations, but enabling this on non-vsx subtargets is different from what GCC does. I don't see a good reason for us to depart from that.

In D91050#2472648, @nemanjai wrote:

Hmm... I think it is very surprising if DAG.getConstantPool() does not return ConstantPoolSDNode. I am not in favour of any design that violates such seemingly reasonable assumptions.

In D92083#2472646, @nemanjai wrote:

Please explain in the description of the review (and subsequent commit message) why custom lowering is needed. I suppose for SELECT_CC, it is needed because using setOperationAction() uses the result type which is only guaranteed to match the true/false value but not the comparison types. However, it is not immediately obvious to me why we need custom lowering for BR_CC.

Gentle ping...

Is it better to have the check before 'if (PDT->dominates(To, BB)) {' ?

Rebase the patch again.

Re-implement with SelectionDAGTargetInfo.

The dependent patch changed. Need rebase.

In D91050#2469885, @RKSimon wrote:

What should we do with SelectionDAG::getConstantPool? At the very least I'd expect better comments explaining when each should be used, but I'm wondering whether we can get away with just the TLI version?

Thank you for the review and Merry Christmas !

In D91053#2467539, @nemanjai wrote:

In D91053#2467200, @steven.zhang wrote:

The test you are looking for is llvm/test/CodeGen/PowerPC/constant-pool.ll in fact.

Not quite. There seems to only be float/double/double double/vector and only for P9/P10. What happens with:

1. enough constants that the load is widened

Address comments and add tests in llvm/test/CodeGen/PowerPC/constant-pool.ll to show the padding added for alignment. Also, add the RUN line for Power8 as requested.

Yes, test is missing as MaskRay mentioned. You'd better update one PowerPC test at least so that, we can see what it looks like.

In D91053#2467480, @nemanjai wrote:

In D91053#2467198, @steven.zhang wrote:

In D91053#2466091, @nemanjai wrote:

In D91053#2465584, @steven.zhang wrote:

There are two parent revisions that this patch depends on. You need to apply them first. I will add a new test to summarize all the pattern we can handle.

Which ones? I don't see them listed in "Parent Revisions"

The test you are looking for is llvm/test/CodeGen/PowerPC/constant-pool.ll in fact.

In D91053#2466091, @nemanjai wrote:

In D91053#2465584, @steven.zhang wrote:

Rebase the patch.

Huh? Something wrong with this rebase? Not only is there an issue with getSize() I pointed out, but there are also uses of non-existent functions:

PPCISelLowering.h:740:61: error: only virtual member functions can be marked 'override'
                            unsigned TargetFlags = 0) const override;
                                                            ^~~~~~~~
PPCISelLowering.cpp:2997:12: error: no member named 'getConstantPool' in 'llvm::TargetLowering'; did you mean simply 'getConstantPool'?
    return TargetLowering::getConstantPool(C, DAG, NewAlign, VT, Alignment,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           getConstantPool
PPCISelLowering.cpp:2987:28: note: 'getConstantPool' declared here
SDValue PPCTargetLowering::getConstantPool(const Constant *C, SelectionDAG &DAG,
                           ^
PPCISelLowering.cpp:3018:12: error: no member named 'getConstantPool' in 'llvm::TargetLowering'; did you mean simply 'getConstantPool'?
    return TargetLowering::getConstantPool(C, DAG, NewAlign, VT, Alignment,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           getConstantPool
PPCISelLowering.cpp:2987:28: note: 'getConstantPool' declared here
SDValue PPCTargetLowering::getConstantPool(const Constant *C, SelectionDAG &DAG,
                           ^
3 errors generated.

Also, I don't see adequate testing here (it may be hidden among all the test case changes and I missed it). There should be a test case with multiple constants loaded from the constant pool for all the types as well as for mixed types. AFAICT most of the test changes are single constants getting their own symbol in the TOC - which neither adequately tests this feature nor provides improvements of any kind (as pointed out in the description).

In D91053#2459632, @qiucf wrote:

If there is only one constant, we will have one extra load with this patch. But the load could be optimized by linker if it merges the TOC. It is not easy inside compiler to handle it as ISEL is done basing on per BB, and we don't know if there are other constants until other BBs are selected. Any thoughts ?

As discussed, FunctionLoweringInfo is 'global' to SelectionDAG. Can we take advantage of it to record some global information? FunctionLoweringInfo::set has already scanned over each BB, each instruction.

Rebase the patch.

Done in 385e9a2a047bc0bee13a21a9016763e694a686a3

The new added field LayoutIndex miss to be initialized in include/llvm/ProfileData/SampleProfWriter.h:281 which cause the compiler(clang) warning if with option -Wmissing-field-initializers.
This is the error message:
error: missing field 'LayoutIndex' initializer [-Werror,-Wmissing-field-initializers]