LGTM. I think we should go ahead with this.

Other than the minor nit, LGTM.

I'd typically be OK with these cosmetic changes being made on the commit, but there are a large enough number of changes that I'd prefer to see the updated patch. Thanks for fixing these.

LGTM. Thanks for the updates.

A couple of notes:

• This doesn't just handle parameters, but return values as well
• I think all of these will require predication for P9Vector since we don't want to change the fp128 calling conventions on subtargets that don't support the type

Other than the formatting issues that Lei identified (and some issues with commas that separate operands not being followed by spaces), this seems fine to me. However, a couple of suggestions:

• Find a reviewer for the target-independent stuff (perhaps from CODE_OWNERS.txt).
• Add a note in the patch description about any documentation of these builtins from GCC and any other compilers
• Actually link the patch dependencies in Phabricator between the clang and LLVM portions

LGTM.

Rebase the test cases with new sigils. Add an assert for an immediate that changes from zero to non-zero in transformation.

Updated the test to include invalid (non-splat) values as well as undefs as part of the splat.

In D44548#1116753, @efriedma wrote:

AArch64 doesn't have a nabs instruction; orr is probably faster than fabs+fneg (unless some implementations have a transition penalty between int SIMD and float SIMD). But we could accept fneg(fabs(x)) as the canonical form and pattern-match it.

Ping.

Ping.

Ping. Do any other targets care about this? The sign manipulation instructions are definitely faster on PPC.

Ping.

Change variable names as requested as it cleans up the code.

I'll let Hal chime in here, but I'd prefer if we just refactor this code to use a range-for instead of this while loop (in all 3 cases).

LGTM.

LGTM. Feel free to address the minor nit on the commit.

In D46761#1096460, @sfertile wrote:

Minor nit: the instructions that use this class are either Z23Form_7 ( xsrqpxp) or Z23Form_8 (xsrqpi, xsrqpix). The structure is correct but I think we should stick with the naming that matches the instruction descriptions in the ISA.

I think we should review the code sequences for signed conversions and make more consistent use of loops.

Thanks for addressing everything so quickly. LGTM.

LGTM.

LGTM.

The code changes required are minor, but there's a few so I'd prefer to see another review of this patch.

If the definition of the new instruction can be avoided, feel free to do so on the commit. If it can't, add a comment as to why it is necessary in the code.
Other than that, LGTM.

@syzaara Do you plan to update this to address the comments?

Added a check for irreducible CFG within the loop being considered for transformation - thanks for the pointers @efriedma and @hfinkel.
We simply bail out of the transformation if the loop has irreducible control flow rather than analyzing said control flow. This results in a total reduction of 2 loops out of 25,432 that are transformed in a bootstrap build.

Thank you so much for your patience with this long review cycle. Aside from a few minor nits, LGTM.
I also imagine you'll need something for the P9 scheduler since we've now marked the model as complete. Probably something along the lines of:

Index: lib/Target/PowerPC/PPCScheduleP9.td
===================================================================
--- lib/Target/PowerPC/PPCScheduleP9.td (revision 331260)
+++ lib/Target/PowerPC/PPCScheduleP9.td (working copy)
@@ -35,8 +35,9 @@

LGTM. Of course, after the dependent patch lands.

I've produced a test case that closely resembles the original CFG. Also updated the condition to hopefully address the valid point that @efriedma brought up.
Please let me know if this is sufficient.

I think this flows much more cleanly now. And if you reorder it to not create the bool temporary and return early, I think it'll be even better. Feel free to do that on the commit, I don't think this needs another review cycle.

Sorry, I find the code kind of difficult to follow now. This is exacerbated by the fact that we end up creating nodes with AddOpcode that produce unary operations - which is very counter intuitive. I think it would be much easier to follow if you made it flow more naturally. Perhaps something along the lines of:

MachineSDNode *flipSign(...)
  if (Type == v4i32)
    <produce and return XVNEGSP>
  if (Type == v8i16)
    <produce the { 0x8000, 0x8000, ... } vector> // The implicit CSE in the DAG will ensure we don't get multiple nodes
  else if (Type == v16i8)
    <produce the { 0x80, 0x80, ... } vector>     // The implicit CSE in the DAG will ensure we don't get multiple nodes
  if (InputOp == Zero)
    <return vector from above>

Other than a couple of minor nits, LGTM.

This seems perfectly fine. Maybe the test case can be slightly improved. Adding the originator of this code to the review.

LGTM.

I think overall, it would be good to explain the semantics of this patch. Maybe even provide a reference to a document that explains it if it isn't something that can be summarized into a comment in the review (or a comment in the code).

I imagine the constant materialization and splatting direct moves would not cost anything additional to what I suggested in an amortized sense (i.e. if LICM takes them out of the loop). However, I think it's still useful to produce code with lower path length.

In D45563#1069984, @spatel wrote:

...
$ ./llc -o - vsel.ll -mtriple=powerpc64le

	xxland 0, 34, 36
	xxlandc 1, 35, 36
	xxlor 34, 0, 1
	blr

xxsel 0, 35, 34, 36 ?

In D45563#1069916, @spatel wrote:

Don't know what changes are planned here, but this is on the right track. We want to have coverage of the possible canonical IR variations for various targets.

PowerPC with Altivec has a vsel instruction if you want even more coverage, but I don't think the PPC backend has the isel pattern-matching logic to produce that currently (cc @nemanjai).

From the PPC perspective, this is fine since the sequences are equivalent. It also seems perfectly reasonable to look through a free operation here.
However, I imagine that @escha might want to look at this as well and give the final approval.

LGTM. Feel free to address the nit comments on the commit.

I'm afraid we can't just do this without doing some more testing. This patch will indiscriminately replace the existing Post-RA scheduler for all PowerPC targets, yet we've only tested it on Power9.

Common PPC code LGTM. However, you might want to request a review for the scheduling portions from someone that knows about the CPU (perhaps @joerg).

This will certainly need another cycle to at least account for the recent changes in PPCInstrInfo.cpp.

Unfortunately, I am afraid I can't really contribute any useful feedback on this patch since I know nothing about the e500 family of CPU's. Perhaps @joerg is the only one that can meaningfully review this patch.

This is much easier to follow now that it is much smaller. Thank you for breaking it apart.

In D44921#1056672, @spetrovic wrote:

-mbss-plt is currently default in LLVM, once secure plt support is finished we can set secure plt as default in LLVM, but not for now.

LGTM. I have tested this on PPC64BE bootstrap and the previous failures are gone. Also, can you please add the test case I sent you as a part of this patch? Clearly, we had missing coverage there.

LGTM.

Add the missing test case updates.

I assume that there are cases where the frame index that doesn't have an offset actually ends up being an address with some displacement and that's the purpose of this patch. What I mean is that I assume that this will sometimes lead to something like:

li 4, 16
stxvx 2, 3, 4

Since we'd never have something like:

li 4, 0
stxvx 2, 3, 4

as that is simply:
stxvx 2, 0, 3

I think overall, we want to cover all the ways we can get the input integer:

• Extending load from a smaller type (only unsigned forms for byte and halfword, both signed and unsigned for word)
• Extracting from a vector (with sign extensions as needed)
• Smaller types in registers
• An fp to int conversion
• A bitcast

LGTM.

In D44921#1050299, @joerg wrote:

GCC supports -mbss-plt to get the legacy behavior. Not sure if anyone actually uses it though.

Hi Galina, do you think you'll have time to commit this soon?

In rL328723#244598, @rnk wrote:

Are you sure this is the right commit? It seems unlikely to be the cause.

This appears to have broken most of the PPC build bots. A couple of examples:
http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/15659
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/16973

Overall I'm very happy with how this patch looks now. The cleanup really allowed this patch to flow much more clearly. However, I think you still haven't addressed the comment https://reviews.llvm.org/D42590#inline-374441

Thank you Galina. I don't have commit access to this repo, would you be able to commit this for me?

I'll let Justin give the actual ACK, but this looks fine to me. The only question that I have (since I don't know anything about secure PLT) is whether this is a PPC-specific thing (since the option is a PPC option).

Added the missing check for the record-form "and immediate shifted" as compares they feed are often redundant.

Ah, OK. The language reference clearly states that unrepresentable values produce undefined results. As far as PPC is concerned, this LGTM.

In D44909#1048805, @scanon wrote:

Two questions, to which I do not know the answer:

(a) Are the semantics of ISD::FP_TO_[US]INT for out-of-range values specified anywhere? The do not seem to be, but maybe I'm just missing it.
(b) Are there test cases that make sure this transform is *not* applied for armv7 and x86 -sse4.1?

Just out of curiosity, do we want to separate out the declaration and definition so that no definition is available in NDEBUG builds? Namely, should the def be something along the lines of:

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD void LatencyPriorityQueue::dump(ScheduleDAG *DAG) const {
  // ...
}
#endif

LGTM.

Thank you for your patience with this review cycle and for cleaning this up. It looks much better now and it's really nice not to have to maintain lists of opcodes in multiple places that have to be kept in sync.

Oh, sorry. I realized just now that you probably fixed this in r328234 and it's just that this bot takes a while to rebuild. I noticed that it has just passed the link step on the latest build. Sorry for the noise.