@jedilyn Hi Ke Wen, thanks for your comments. This needs some cleanup with regard to which instructions match the semantics of F{MIN|MAX}NUM vs. F{MIN|MAX}NAN. I'll clean that up and re-post this for your review. Thanks.

Since there are unaddressed comments, I'm moving this off of my Must Review queue until they are addressed.

Please add a test case that we used to transform to a D-Form and no longer do with your patch (preferably one for DS-Form and one for DQ-Form).

Please provide the patch with full context as per http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. Furthermore, if you would like to commit this, it'll have to go into trunk and then it can be merged into 7.0.1 (I'm not sure that we'd be able to merge it into any 6.0 release as there may not be a 6.0.2).
Finally, for the test case you can just compile anything with llc with the triple you are interested in and a CHECK directive can check for .abiversion 2 in the assembly.

LGTM.

LGTM.

LGTM. For a change such as this, I like the idea of committing a test case prior to the patch so the patch clearly shows the difference. Thank you.

Minor nit, but LGTM.

LGTM.

I think this patch caused failures (in addition to existing ones) on the PowerPC Big Endian buildbot (http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/23397). It fails unit tests in the file added in this patch, which is what makes me suspect that it is this patch that causes the failures. Can you please investigate or let me know if I need to do something on the buildbot (clean up directories, etc.)?

Hi Dean, I think this patch may have caused failures on the PPC Big Endian build bot and the failures were hidden by other build breaks that happened near the same time. Initially this caused failures at compile time which you fixed in r341042. However after that was fixed, we are now getting failures that I don't understand but certainly seem related to this patch (please refer to: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/23322).
The failures being:

FAIL: LLVM-Unit::FDRTraceWriterTest.WriteToStringBufferVersion3
FAIL: LLVM-Unit::FDRTraceWriterTest.WriteToStringBufferVersion2
FAIL: LLVM-Unit::FDRTraceWriterTest.WriteToStringBufferVersion1

In D51353#1220682, @inouehrs wrote:

LGTM with nitpicking.

I feel that the current test case is somewhat fragile for minor change in inst. scheduling or register allocation.
Checking there is no cmpdi between andis. and isel may be enough.

LGTM aside from a minor nit which you can fix on the commit.

LGTM. Thanks on working on completing the set of itineraries.

@inouehrs I think I've addressed your comments adequately. Do you have any further comments or can you accept this revision?

@inouehrs Can you please review/comment/approve this as I've separated this out per your request.

In D44897#1215313, @inouehrs wrote:

This patch increases the number of record-form rotates we emit by more than 70%.

Do you mean reducing the record-form rotates by 70%? If so, it is great!

Oops! Yes this patch definitely causes a reduction in the number of record-form rotates :)
I updated the description (it was meant to say eliminate rather than emit).

Remove the orthogonal change that allows us to optimize compares fed by record-form ANDIS.

Ping. I'd like to get this committed. Can someone please review?

It would appear that this patch caused buildbot failures. An example at: http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/19586

This seems to make sense to me and it resolves the kernel build issue. If @sfertile sees something wrong with the patch, he can chime in before or after the commit.

Other than the minor nit about the test case, this LGTM.

In D49007#1203739, @steven.zhang wrote:

I have done the following tests with this patch:

1. LLVM check-all uniitests
2. LLVM test-suites
3. bootstrap build the clang with the clang including my fix, and then, run the llvm check-all and test-suites, and it passed.
   
   The patch passed all these tests.

LGTM aside from the minor nit.

In D49531#1198338, @wuzish wrote:

Could someone commit for me since I don't have svn access. Thank you very much.

For whatever reason, this does not seem to be NFC. It causes a number of failures on the PowerPC targets (including buildbots). For an example, please consider: http://lab.llvm.org:8011/builders/clang-ppc64le-linux/builds/19269

Is this patch ready to be abandoned now that it has been broken up into smaller patches which were committed?

LGTM.

LGTM. Since we're all in agreement that we want to make this change for Power9, I think this is fine to go in.

LGTM.
Perhaps @thegameg or @MatzeB want to look at it as well (particularly the target independent parts). And perhaps @hfinkel and @echristo are interested in having another look. Otherwise, I suppose this is ready to go.

LGTM

This is definitely going to need a test case.

LGTM.

I think that this is semantically correct now. There are just a few other stylistic comments.
Also, since this is a rather high-risk patch, can you describe on the patch the functional testing you have done with the patch (list of applications, bootstrap, etc.)?

In D50004#1195212, @jsji wrote:

In D50004#1194086, @jsji wrote:

<snip>

diff of assembly before change and after change:

$ diff -Naur before.s after.s 
--- before.s    2018-08-09 13:52:24.785246846 -0400
+++ after.s     2018-08-09 13:59:38.815493708 -0400
@@ -9,7 +9,7 @@
 # %bb.0:                                # %entry
        lfd f0, 304(r1)
        lxsd v2, 312(r1)
-       xxlor v3, f1, f1
+      xscpsgndp v3, f1, f1
        xsmaddadp v3, v2, f0
        xsadddp f0, v3, f2
        xsmaddadp f0, v3, v2

This is a rather non-controversial change that resolves a release blocker bug. I'm going to commit this fix later today.

In D50004#1187625, @jsji wrote:

<snip>

No sure how much this will have impact, but maybe we need to consider still using xxlor for destructive instructions?

eg:
In Power ISA 3.0 B, 2.1.5 Destructive Operation Operand Preservation
"The set of instructions listed below, when immediately preceded by the xxlor XT,XC,XC instruction in a sequence similar to the above example, will provide optimal performance."

In D50004#1182972, @inouehrs wrote:

XSCPSGNDP has longer latency (6 cycles) than XXLOR (2 cycles) on POWER8 while it has higher throughput with the same latency on POWER9. So XXLOR is preferable for pre-P9.

Also, the two instructions have different behavior for a denormal input value in my understanding; XSCPSGNDP does normalization but XXLOR does not. Does this difference matter?

+1, even for Power9 XSCPSGNDP makes pipeline busy longer than XXLOR, XXLOR is still better.

Can you clarify this please? Where is this information coming from? According to the UM, XXLOR takes up a whole superslice whereas XSCPSGNDP takes up a single slice so we can dispatch 2 of the former per cycle and 4 of the latter. And the "Pipe Busy Cycles" field for both is 1.

Ping. The bug this resolves (38087) is marked as a blocker for 7.0. @hfinkel Can you please have a look at this patch when you get the chance?

In D50121#1183765, @tstellar wrote:

One questions I had after looking into this bug was why does PPCTargetLowering::LowerFP_TO_INTForReuse() extend f32 values to f64 before creating the FCT* instructions?

Place comment into the respective if block.

LGTM.

There are a few minor nits, but aside from those this LGTM.
Also, please make note of the fact that we need to fix sequences such as xxsldwi -> xxspltw and xxpermdi -> xxspltw in PPCMIPeephole.cpp.

Forgot to approve the patch.

I don't see anything in the update that would cause me to reverse the approval, so I'd say this is still fine.

This needs to be beefed up to account for the two unsafe situations I outlined in the comment.

LGTM.

A number of the registers in the test cases you've added are mandated by the ABI. Rather than keeping track of which ones should be part of the test case, perhaps it's easiest to use utils/update_llc_test_checks.py to automatically generate the CHECK directives (if it works for all the RUN lines in your tests).

There's a notification from Phabricator that says:
This file was changed only by adding or removing whitespace. Show File Contents
for file lib/Target/PowerPC/PPCISelLowering.cpp. If this is a glitch with Phabricator, fine. Otherwise please remove any such unrelated changes.

Thank you for updating the code to get the CSR list from RegInfo. It looks much cleaner than having a static list of registers. I think the code can be cleaned up a bit further still, but overall this looks great.

LGTM.

LGTM.

LGTM.

LGTM.

Other than a few style nits that can be fixed on the commit, this LGTM.

Perhaps my comment about the GCC documentation really belongs in this patch rather than in D48185.

Can you put a link in the description of the patch as to where the GCC builtins are documented (if such a document exists). The naming seems inconsistent and I don't see it at https://gcc.gnu.org/onlinedocs/gcc-8.1.0/gcc/PowerPC-Built-in-Functions.html.