LGTM.

You might want to avoid adding new check prefixes in the tests if the sequence is exactly the same. Maybe just change it from CHECK-P9 to CHECK-P9UP. Otherwise LGTM.

LGTM. The PPC changes are fine AFAICT.

In D114656#3159300, @MaskRay wrote:

PPC64R12SetupStub is not only used by NOTOC->TOC code, but is also used by NOTOC->NOTOC long branches, so __long_branch_pcrel is not that bad. That said, if __gep_setup makes more sense, I am happy to use it, too.

Seems fine to me but maybe give @MaskRay a couple of days to see if this adequately addresses his comment.

LGTM.

My comments are just minor nits that don't require another review and can be addressed when committing the patch.

In D114654#3156432, @sfertile wrote:

Nice work Yousuf. Can you start by adding the context for the patch (for example if you used git diff or git show then adding -U9999 will give you 9999 lines of surrounding context in the diff), and running clang format on the patch?

LGTM.

How important is it for the thunk symbol names to match ld.bfd/ld.gold?

LGTM. Thanks for the update.

LGTM aside from a name and return type change.

Please also add a test for this builtin to the front end test clang/test/CodeGen/builtins-ppc-crypto.c

Just out of curiosity, what does the test case expect? I see mention of Trap() causing a zero vs. non-zero exit code. I don't really follow that since a trap is a void function that doesn't return. But I suppose this is referring to the exit code from the program which I imagine will be something like SIGABRT. Should the invocation of not in the test case just have --crash?

Just out of curiosity, if the csect that contains a function is aligned at 2 bytes, how do we ensure loops within those functions are aligned at 32 bytes?

Although the "one in 10" estimate seems to be a bit inflated, the test case certainly seems to be flaky so I am not opposed to disabling it. Thank you.

This LGTM but lets give it a few days for others to chime in with their opinion.

In D114122#3139130, @MaskRay wrote:

Add a new CMake variable similar to LLVM_OPTIMIZED_TABLEGEN? I think most people don't need comments even for their !NDEBUG builds.

LGTM. Thank you.

Thanks for adding these. I look forward to seeing improvements to some of these as there are a number of them that currently produce fairly terrible code.

LGTM other than minor nits.

Please rebase. Does not apply cleanly to ToT.

Please provide a description for this patch which includes justification for why we want to allow conversion between the two types.
I am of the impression that allowing the two types to coexist in completely disjoint code should be fine, but I really don't see a compelling reason to allow conversions between the two types.

LGTM. There are some very minor nits that can be addressed on the commit.

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

% readelf -a ../src/sys/amd64/compile/X/modules/usr/home/kostik/work/DEV/src/sys/modules/ufs/ufs_vnops.o | grep GOT | head -3
0000000001f7 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
00000000060d 004a0000002a R_X86_64_REX_GOTP 0000000000000000 stack_chk_guard - 4
0000000007c8 004a0000002a R_X86_64_REX_GOTP 0000000000000000 __stack_chk_guard - 4

Maybe this should have an additional check for freestanding mode?

In D113178#3111575, @jsji wrote:

...
But this does introduce confusion about register classes -- it is suspicious that xxspltd operate on f0 instead of vs0.

Ha ha, yup! Confusion about register classes is kind of a fact of life with PPC's complex overlaying of registers in a single register file. It certainly takes some time to get your mind around FP/VR/VSR/ACC registers.

I am not in favour of this patch. The reasons I added XXPERMDIs a long time ago are:

1. To allow a single input operand for single register splat/swap. This is useful when the input is a load (since due to chains, having a load as an input will end up with both loads emitted - i.e. no CSE).
2. Since this is primarily useful for loads that load a partial vector (LFIWZX, etc.) the input register class is vsfrc (i.e. all scalar floating point registers).

LGTM other than a number of stylistic changes. Feel free to address those on the commit. You also might want to give @amyk a bit of time to ensure her comments were adequately addressed.

I believe you are planning an update for this patch. Requesting changes to take it off the queue until you have uploaded the updated version.

LGTM. Thanks for fixing this.

Seems like an obvious fix. LGTM.

FWIW, the PPC changes look fine.

LGTM.

In D111433#3061400, @qiucf wrote:

Thanks for fixing this!

I guess the reason to restrict them only undef strict-fp is performance?

Right, preventing optimization around calls due to rounding mode without -frounding-math would be unnecessarily restrictive for most compilations.

LGTM. This is already approved, but just wanted to indicate that I think the comments were addressed adequately.

Please change the condition as Jinsong suggested and change the leading comment to:

// On AIX, only emit the extended mnemonics for dcbt and dcbtst if
// the "modern assembler" is available.

Significantly simpler. LGTM, thank you.

Please try out the simpler alternative of letting the legalizer take care of the store size. If that alternative is not possible, please add comments to the patch explaining why it is not.

LGTM as long as the sign-extending test is added.

LGTM. Please run clang-format on the patch (only the modified lines).

LGTM as long as you add the back end test.

LGTM with the test nit addressed.

@saghir Since the bots have been failing for a few days now and there is no response, please try to figure out what commit this is a fix for so we can revert all related commits until we can get to the bottom of this problem.

LGTM. Thanks.

LGTM.

I may be wrong, but I really think this is incorrect. Please do some functional (execution) testing on this. Also, please re-title this from "truncate results" to something like "truncate exponent parameter" or similar since you are not truncating the result but the parameter.

In D110631#3028852, @saghir wrote:

Hi,

This change seems to be causing issues with Power PC buildbots. One such bot is https://lab.llvm.org/buildbot/#/builders/52/builds/10906. Can you please take a look to fix/revert the change? If there is anything I could do to help investigate, please let me know.

Thanks!

Added @jansvoboda11 to the review as it appears he was the one that added the original option.

LGTM. Please note in the commit message that this is simply a wrapper for a floating point divide. XL provided this builtin because it doesn't produce software estimates by default at -Ofast.

LGTM.

LGTM. Thanks.

I really don't understand what happened now. It seems that you have simply reverted to an older version of this patch. The test case appears to not have been pre-committed any longer, the Power9 patterns and test cases are gone. What has happened here?

LGTM.

Thank you for fixing this.