Ping.

Thanks @jsji and @nemanjai for your insights on this patch. I will abandon this for now.

I've realized I forgot to attach a full context patch.
Updated with full context.

I've realized I forgot to attach a full context patch.
Updated with full context.

I think this also LGTM overall.

I think this overall LGTM.

I think this overall looks good. Thanks for extending this to handle the load->shuffle case!

Thanks @jsji for letting me know, and thanks @nemanjai for the handling of the load and splats!

• Rebased to the latest source (fix conflicts, ran functional tests)
• Fixed llvm/test/CodeGen/PowerPC/ppc64-P9-setb.ll to remove redundant sign extend instruction
• Added the following check in PPCMIPeephole.cpp to only perform the LWZ->LWA transformation when the displacement is a multiple of 4

// The transformation from a zero-extending load to a sign-extending
// load is only legal when the displacement is a multiple of 4.
// If the displacement is not at least 4 byte aligned, don't perform
// the transformation.
if (SrcMI->getOperand(1).isGlobal() &&
    SrcMI->getOperand(1).getGlobal()->getAlignment() < 4)
  break;

Hi @inouehrs, I will be commandeering this revision since we are revisiting this patch.

LGTM.

This looks good to me.

I think this LGTM.

Updated the compiler_rt_logb_test.c to:

• run on non-GLIBC systems
• consistent use of _GLIBC_PREREQ macros in compiler-rt

With the assistance of @hubert.reinterpretcast, I have updated the sanitizer-common test case, getpw_getgr.cc to avoid the use of std::string as this can result in a false positive.

@rupprecht Thanks for the insight regarding compiler_rt_logb_test.c. I have updated the patch slightly, so I am no longer modifying the other compiler-rt builtins test. Does this update look more suitable? If you're OK with the current logb test, I can just keep it as is.

Originally, this patch modified the following to check for the version of GLIBC:

• Compiler-RT builtins test cases: compiler_rt_logb_test.c, divsc3_test.c, ppc/qdiv_test.c
• Sanitizer test case: getpw_getgr.cc

@rupprecht That is true. However, isn't it possible that a system's most up to date GLIBC version is still lower than say, GLIBC 2.23? Starting a conversation on which GLIBC versions are important can possibly be something that can be done.

@hubert.reinterpretcast Thank you for reviewing and bringing up these concerns, I do understand and you do have a point.

I have forgot to attach the full context of my initial patch. I apologize for this.

@hubert.reinterpretcast My apologies, you're right, I forgot to upload the full context. I will do that first, and address your comments afterwards.

I think this still looks good to me, with the addition of the documentation.

Looks good, thank you for fixing and adding the test case.

This looks good to me.

Hi, I believe this is causing Buildbot failures for PowerPC: http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage

This is causing buildbot failures for PowerPC: http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage

In D54313#1356885, @nemanjai wrote:

Amy, can we get this committed?

Updated patch to address:

• renamed variables for names that adhere more to LLVM naming conventions
• changed the low part of the function argument to unsigned, as it will later be passed into a function that expects an unsigned argument
• Removed a temporary that represented 2^64.

Updated this patch with:

• initialization of int128 macro to simplify test cases

Updated this patch to address:

• Updated comments that are more descriptive
• Renamed variables
• Removed the macro originally used to group up common code. Since the code that seems duplicated is not entirely identical, the code will remain duplicated as utilizing a macro still requires an if-condition to control the conversion depending if we are converting and high double or a low double

Updated patch to address @renenkel's review :

• Renamed union type to represent the long double
• Updated comments
• Updated variable names
• Updated algorithm to be more concise and succinct

Previously uploaded the wrong diff by accident.

Updated this patch with:

• Renamed variables
• Utilized compiler-rt specific functions (like NaN)
• Added the long double union structure in the same file as implementation (containing high and low doubles, high and low bit patterns)

Updated this patch to reflect:

• Changed variable names;
• Removed test cases with INFINITY and NAN
• Added the long double union structure (containing high and low double portions, and their corresponding bit patterns) to be used in the testing file (to remove the dependency of looking at two files for the implementation of the long double)

Are there any more comments regarding on if all of these tests should contain COMPILER_RT_ABI (and the relevant include statement) if all compiler-rt functions should have them anyway?

@dschuff Is it possible to elaborate on why COMPILER_RT_ABI is needed in floatditf.c and floatunditf.c, and/of if it still needed?

@weimingz I noticed that you have previously added the REQUIRES and RUN lines to these tests, and have added you as a reviewer in regards to me updating the lines you have previously added.

@renenkel Yes, thank you for pointing that out. I accidentally mixed that up, my apologies. The summary has been corrected to reflect the correct behaviour.

Updated the diff to since a specific test on line 528 in vsx.ll now using the v4 register instead of vs0.

Updated the test case again, removing the assert line as previously addressed.

Updated the patch to address:

• Making the test cases a little bit less exhaustive (removing CHECK-NEXT's)
• unnecessary whitespaces

This revision primarily addresses enabling xscpsgndp for P9 only, as xxlor is still preferable to use for copying floating point scalars pre-P9.

Further update of test cases prior to commit.

Updated revision to remove irrelevant whitespaces.