Just some minor comments, I think it overall LGTM but am curious in knowing any comments from others.

Just curious about a specific portion of your patch, so I posted a comment.

One minor comment from me. I also agree with what has been stated previously (to try to simplify tryAndWithMask()).

This also LGTM.

@tstellar Yes, I will commit this patch and also open a bugzilla for back porting.

Does anyone else have any specific comments regarding this patch?

LGTM.

This also LGTM. Thanks for fixing.

I have the same nits as @NeHuang, but this overall also looks good to me.

Updated with more specific comment.

Updated with appropriate diff.

The intention was simply to fix non determinism when building on RHEL, and the version checks were simply to make sure that this patch only gets applied to versions of ar/ranlib that support it.

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.