I think this is the right thing to do regardless of whether it affects CTR loops or not. The FMA is never faster with SPE and that should be marked as such. But yes, we still need a test case.

Please run clang-format, rebase and re-upload. It doesn't apply cleanly to ToT.

LGTM. Thanks for fixing this.

As tends to be the case with huge patches such as this, it is difficult to get this patch to converge and it still needs a bit of work. But I think it is pretty close overall.

"Made a new phabricator review because of git issues" is not an appropriate description of a review/revision.

I am ok with this change overall, I just have a couple of questions about naming of the option.

Is it possible to add a testcase that fails to verify or otherwise shows that a live range is computed incorrectly?

Ah, this is a good point. I guess we didn't consider that we may have a bitcast of a bitcast. Thanks for fixing this. LGTM.

LGTM. Thanks for fixing this.

LGTM. Thanks.

LGTM. Thanks for the fix and all the updates.

LGTM. Thanks for the fix.

If this IR is compiled with -mattr=+quadword-atomics, we emit #STQX_PSEUDO which is definitely not what we want:

%struct.StructA = type { [16 x i8] }

In D127364#3568733, @vitalybuka wrote:

https://lab.llvm.org/buildbot/#/builders/105/builds/26496
and
https://lab.llvm.org/buildbot/#/builders/19/builds/11181

In D127310#3567472, @MaskRay wrote:

Do you have more authoritative answer when /root/usr is used and when it isn't?

These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.

The original toolchain detection added root/usr to the paths which was certainly necessary on PowerPC. Since that suffix is removed in this patch, our Redhat buildbot is now broken (https://lab.llvm.org/buildbot/#/builders/57/builds/18577). @quinnp has posted an update to this at https://reviews.llvm.org/D127310.

This was never committed because it turned out not to be beneficial. Abandoning.

LGTM.

In any case, this works if the offset fits but not if it doesn't. So we're just kicking the can down the road a little bit. We need to add the X-Form pseudo to the ImmToIdxMap and expand it at the appropriate time.

In D126807#3551902, @lkail wrote:

I think the root cause here is lq/stq is lacking x-form. There are LQX_PSEUDO and STQX_PSEUDO defined to assist accessing big offset inside stack/heap, but they are expanded pre-RA. I think we should expand them in PPCAtomicExpandPass(which is after prologepilog) and make lq/stq's x-form mapping to LQX_PSEUDO/STQX_PSEUDO.

FWIW, this test fails exactly the same way on my MacBook if I use llvm-nm on llvm-config which produces shorter output (although it is successful in its current form). So this seems to be completely pervasive and not at all restricted to PowerPC (or even Linux).

In D124607#3536329, @jhenderson wrote:

Both llvm-symbolizer and llvm-cxxfilt, if called without positional arguments, are run interactively, essentially taking a line of input then writing some output before waiting for more input. This would allow you to force output to be written (by writing to stdin) after the pipe has been closed. Maybe that'll solve the issue, although I'm not sure I understand why the python script doesn't work in its current form: llvm-nm should try to write many bytes of output (> 1 byte), but the pipe is closed after the very first byte is written. Before going and changing the script, I'd like to understand whether this statement is actually true, and if so, why:

That does not appear to be enough. You can either run a tool that runs longer or run llvm-nm with something that produces more output.

The more analysis we do in FastISel, the less "fast" it becomes. And the tradeoff here is simply a nop after a call which hardly seems like a concern at -O0.

This seems like a rather invasive refactoring for removing a nop at -O0. I am not particularly in favour of this. Not only is a nop essentially trivial when it comes to performance (and even code size), but this is restricted to unoptimized code gen for which it isn't really reasonable to expect performant code.

In D124607#3515118, @dmcyk wrote:

@t.p.northover made the commit, but it broke clang-ppc64le-linux-multistage buildbot, so it's been reverted.
In the test the input to FileCheck was empty, so maybe the signal handler didn't trigger for some reason.

The test does work on x86 Linux and macOS, so I've tried a ppc64le VM running CentOS 8.
The VM is too slow to compile the entire llvm, but I've tested a simple program with a SIGPIPE signal handler, the same python script and that worked as expected.
So the failure seems to be specific to RHEL or the buildbot environment. Maybe someone from #powerpc can help test this?

In D125846#3523981, @nemanjai wrote:

In D125846#3523688, @arsenm wrote:

Why can't you just mark these instructions as being terminators? AMDGPU has to glue a lot of ALU instructions to terminators, and we just define terminator and nonterminator pseudo variants

This seems like a less desirable path. Marking what is inherently a non-terminator instruction as a terminator seems likely to cause problems. I don't have a specific case in mind, but perhaps if some pass makes the assumption that it is safe to split a basic block into two that terminate with two terminators. It might be a safe thing to do except if some terminator is not actually a terminator.

In D125846#3523688, @arsenm wrote:

Why can't you just mark these instructions as being terminators? AMDGPU has to glue a lot of ALU instructions to terminators, and we just define terminator and nonterminator pseudo variants

In D125846#3521120, @aemerson wrote:

Can the test be reduced? Do you really need a main()?

In D125203#3502651, @tingwang wrote:

In D125203#3502433, @nemanjai wrote:

Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?

Oh I missed that. Thank you for pointing out!

Just now updated the patch. However I didn't update the SemaFeatureCheck message to indicate the support on a2, since if people see this error message, it cannot be a2, and a2 does not easily fit into the message of err_ppc_builtin_only_on_arch. Hope this will not create problem.

Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?

In D125203#3499836, @lkail wrote:

The troubled instruction was introduced for both P7 and A2 as below link shows:
https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130325/169706.html

This link has been invalid.

In D124841#3499517, @efriedma wrote:

In D124841#3499199, @nemanjai wrote:

In D124841#3489679, @efriedma wrote:

...

Are you saying that even though we have a frame pointer, it's impossible to allocate a stack slot close to the frame pointer? That seems unlikely to me; normal stack layout rules should allow allocating as much space as we need close to the frame pointer. Depending on how the stack is laid out, I guess it's possible that normal spill slots are never close enough to the frame pointer. But if we really need it, we could allocate the emergency spill slot next to the callee-saved registers. (This currently isn't implemented, but shouldn't be complicated.)

I am sorry that I didn't get to this within a more reasonable time. This LGTM for PPC and thanks for the patch.

This is fine from a PPC perspective. What would make more of an impact would be the ability to common partial immediate materializations such as happen in prologue/epilogue when a stack frame is very large. Perhaps this can be extended to do that in the future. It would eliminate things like:

lis 11, 16
ori 11, 11, 256 
stxvx 53, 31, 11                        # 16-byte Folded Spill
lis 11, 16
ori 11, 11, 272 
stxvx 54, 31, 11                        # 16-byte Folded Spill
lis 11, 16
ori 11, 11, 288 
stxvx 55, 31, 11                        # 16-byte Folded Spill
lis 11, 16
ori 11, 11, 304 
stxvx 56, 31, 11                        # 16-byte Folded Spill

Of course, that would involve changing the subsequent ori instructions to addi and would likely be very PPC-specific. I just bring it up here in case SystemZ has the same issue.

In D124841#3489679, @efriedma wrote:

If such an emergency spill also needs to be spilled to a location with a large offset

This shouldn't happen; the emergency spill slot should be placed in a location with a small offset from the frame/stack pointer.

If it's impossible to allocate an emergency spill slot close to the stack pointer, I think we should force a frame pointer, and the emergency spill slot should be allocated close to that. See, for example, AArch64FrameLowering::hasFP().

Thank you for fixing this!

In D124643#3486603, @beanz wrote:

Bots should be fixed with rG55e13a6bc0d6a31afc258a012184253041b0eb8e.

LGTM. Thanks for the fix.

This can go in as it is. Both Clang and GCC define this macro on PPC. However, it will mean that we don't define these even for compilations with -mabi=ieeelongdouble and -mlong-double-64. If it matters that this works when long double means IEEE 754-R Quad Precision or simply double precision, you could guard it with something like the following

#if defined(__PPC__) && defined(__LONG_DOUBLE_128__) && !defined(__LONG_DOUBLE_IEEE128__)

In D124167#3472621, @klausler wrote:

In D124167#3472582, @nemanjai wrote:

In D124167#3471421, @kiranchandramohan wrote:

@jeanPerier I think a buildbot is failing possibly due to this patch. Could you have a look?
https://lab.llvm.org/buildbot/#/builders/21/builds/39420/steps/5/logs/stdio

@jeanPerier Do you have a fix for this issue yet? If we don't get a fix before the end of the day, I'll have to revert this patch.

When is "the end of the day" for you? I might be able to patch this, given some time.

In D124167#3471421, @kiranchandramohan wrote:

@jeanPerier I think a buildbot is failing possibly due to this patch. Could you have a look?
https://lab.llvm.org/buildbot/#/builders/21/builds/39420/steps/5/logs/stdio

In D122983#3465122, @aaron.ballman wrote:

In D122983#3465099, @nemanjai wrote:

@daltenty Can you please run this with the same config as the bot on one of our AIX machines but be sure to pass -i if it's make or -k 0 if it's ninja?

That would be *awesome*! I have to take off for a few hours, but I'm happy to fix all of the issues you spot in one go when I get back in a bit. Thank you!

@daltenty Can you please run this with the same config as the bot on one of our AIX machines but be sure to pass -i if it's make or -k 0 if it's ninja?

FWIW, this is the reduced test from the PPC bot failure:

; ModuleID = 'bugpoint-reduced-simplified.bc'
source_filename = "macroblock-e4628c.c"
target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512"
target triple = "powerpc64le-unknown-linux-gnu"

Also, please run clang-format on the changes.

In D123997#3461035, @umesh.kalappa0 wrote:

@nemanjai ,thank you for quick fix and please checkout the unit test failed ?

In D122983#3463569, @xbolva00 wrote:

But your link shows failures in compiler-rt/, not in llvm test-suite

This is still causing failures when building test-suite:
https://lab.llvm.org/buildbot/#/builders/105/builds/24292

Committed in https://reviews.llvm.org/rG315d79213025

In D116867#3446877, @pkubaj wrote:

@nemanjai
As mentioned by adalava, can you commit this patch?

In D67678#3446526, @rsmith wrote:

@dexonsmith @arphaman What do we need to do to get this re-landed?

While I certainly appreciate the work to refactor this behemoth of a function, I am about as lukewarm on how much of a readability improvement this is. If all the targets are being similarly refactored, I won't stand in the way of doing the same to PPC. So if you get the approval for the other targets, we'll go ahead with PPC as well.

Sorry, I had some comments that I haven't submitted because I had not gotten through the entire patch. I am not sure if they all still apply but thought I'd submit them before restarting the review.

Please add a test case that would cause a failure prior to this patch due to the argument being emitted more than once (i.e. the test case that prompted this patch). If that is already added and I just missed it, please add a note to the test case along the lines of:

// The argument expression must not be emitted multiple times.

I am not familiar with the FreeBSD-specific stuff, but there is nothing I see any issues with here.
LGTM.

LGTM.

Seems like we should look at the caller side as well since we might have a similar issue.

This certainly fixes all the lsan issues on Ubuntu 18.04, leaving only one failure in the CRT test:
CRT-powerpc64le-linux :: dso_handle.cpp

LGTM other than the test cases that need to be improved.

LGTM.

LGTM.

In D120305#3346880, @MaskRay wrote:

While I feel sorry for leaving clang-ppc64le-rhel red for some time and am willing to fix issues if I have access to a ppc64 machine (especially compiler-rt ones that I care about),
I feel uncomfortably if a group just bluntly request "please pull this patch" when apparently (a) there is a better approach (explicitly setting CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer can do
and (c) there is just some inherent stability problem (in this case, consider not enabling the testing when the target is still unstable) that is causing not only this issue, but various other reports (as I watch sanitizer failures quite closely and ppc64 often tends to be the outlier thing)

In D120305#3346144, @MaskRay wrote:

In D120305#3345978, @MaskRay wrote:

In D120305#3345810, @RKSimon wrote:

@MaskRay The ppc buildbots have been red since these patches - please can you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454

Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. I'll just disable them: https://github.com/llvm/llvm-project/issues/54084

Actually I don't know how to disable the tests: https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
Hope someone from #powerpc can disable them.

LGTM.

In D94576#3269996, @lebedev.ri wrote:

In D94576#2611255, @tislam wrote:

Hi @lebedev.ri, did the second example work for you? Please let me know if you need any additional information.

Sorry, this completely dropped off my radar.
My comment is still the same, it would be good to to understand,
is there some particular reason why verification is slow for that sample?
Do you have a bisection as to when it became slow?

Hiding verification under EXPENSIVE_CHECKS is somewhat problematic,
it will effectively disable verification.

LGTM.

LGTM. Thanks for fixing this.

LGTM.

In D116867#3229457, @pkubaj wrote:

Note that __ppc_get_timebase is pretty much copied from glibc (writing it here to make sure that there are no licensing issues).
That said, I don't think there's any other way to read TBR.