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]

That means we have to make it as target independent. I will take a look to see if there is nice way to make it as target independent.

In D93370#2458401, @amyk wrote:

@steven.zhang The idea is that this patch should be NFC. All existing load/store test cases should pass with this refactoring. I do think there should be more tests added, perhaps in a follow up patch. What do you think?

Address comments.