Forgot to select request changes.

A few more comments need to be addressed for this to proceed.

The shift amount for one of the patterns was wrong for little endian.

This introduces a dependency for https://reviews.llvm.org/D72569.

LGTM.

Aside from a couple of minor nits, LGTM.

If I understand this correctly, this just evaluates the query for lock free atomics at compile time if the size is larger than the maximum possible size for an atomic on the target. If that's the case, this looks fine to me. But of course, some of the other target maintainers might feel otherwise.
The incompatibility with GCC might be an issue for some, but I don't expect this to be an issue at least on PPC.

Adding Florian, Daniel and Craig as reviewers since they were involved in the most recent change to this area of table gen.

Forgot to accept.

LGTM other than a minor nit that can be addressed on the commit.

LGTM.

The code looks fine to me, but where is the test case?

Thanks for removing this. LGTM.

Missing context on this review. Please produce the diff with -U99999 or arc to ensure full context is available.

Other than a few minor nits, LGTM.

The remaining comments are stylistic nits that can be addressed on the commit. Assuming that those will be fixed, LGTM.

In D68411#1722968, @anil9 wrote:

In D68411#1718116, @lei wrote:

Since these are just mnemonics for using dcbf, why not just add the tests to llvm/test/CodeGen/PowerPC/dcbf.ll instead of creating 2 small test files.

These are mnemonics true, but we have separate intrinsics for them as opposed to using one intrinsic with different constants and also the assembly code is different.
I think having multiple files gives more structure and in theory reduces chances of merge conflicts.

I realize that without any uses of these functions, we can't really add a test case. However, can we at least get a description of why this is useful in the middle end if this patch won't add any users of these functions?

I think this refactoring doesn't really do anything to simplify the code or make it more readable. It just takes fairly complex logic and moves it into a separate function. Then subsequent patches will add even further complexity to this function. Overall, there isn't much of a gain from the refactoring.
What I think would make sense is to simplify the tryAndWithMask() to something conceptually similar to the following:

tryAndWithMask(...) {
  if (canBeCodeGenedAsSingleRLWINM())
    return produceRLWINMSDNode();
  if (canBeCodeGenedAsSingleRLWIMI())
    return produceRLWIMISDNode();
  if (canBeCodeGenedAsSingleRLDICL())
    return produceRLDICLSDNode();
  if (canBeCodeGenedAsPairOfRLDICL())
    return produceRLDICLPairSDNode();
  ...
  return SDValue();

The code you are modifying should be unreachable and should be safe to remove. However, can you please provide a description with some rationale about why you are removing this now. Namely, is there a pressing concern to get this removed soon or is this just a regular cleanup?

LGTM but maybe give the original author a day or two to have a look as well before committing.

Run git-clang-format.
I don't want to address the clang-tidy check results as I don't really want to change the induction variable from i to I. The former is ubiquitous.

I would say that this is as close to a [perhaps begrudging] consensus as we'll get. Lets get this bike shed built. Please go ahead with the commit. LGTM.

I have pushed the other patch now. Please rebase this patch against ToT.

Add a comment for the patterns.

Add the feature to the BG cores as well.

This should fix https://bugs.llvm.org/show_bug.cgi?id=39865

In D71287#1797468, @jhibbits wrote:

The conflict I see is the other patch uses VSX for the operations, but this uses instructions that have been around as far back as the PPC970.

In D65967#1730540, @ajwock wrote:

Sorry for the delay- I had to investigate some of the fragility so that my test case reduced to this pass would still trigger the issue. However, I did manage to get a test case that is only a few lines long that would trigger the issue without the update.

This seems straightforward to me - I assume the constant pool load in the test case is some value denoting the smallest normal value for the type. Then rather than comparing against zero do determine when to return zero, we compare against that value (after taking the absolute value).

Please refer to https://reviews.llvm.org/D69949 that is about to be committed. I haven't looked at this patch but it seems related/overlapping. If there is an unforeseen issue with the other patch, please comment there.

I committed https://reviews.llvm.org/rGba5a00167bf30df5d544fdbe9fd28ce1a8341b89 to fix the bots. Please review that patch. If the fix is not quite as desired, please update it but I wanted to make sure to commit it to get the bots back to green.

This commit (or one of the related ones in the series) has caused all the build bots to fail.

LGTM.

Other than the minor nit about readability, LGTM.

Something doesn't match up here. The description/title say that you are changing the flag for both MFLR/MTLR. But in the code I only see the change for MTLR. Why is that?
Also, the context is missing from this patch so it cannot be reviewed properly, please fix that.

In D71113#1772573, @avl wrote:

Hi @nemanjai I has recently integrated the fix for that problem https://reviews.llvm.org/rG8c714c93023d7d039a23fb47c8256570ba54b9c7 . I am sorry for inconvenience.

This breaks the shared libraries builds. An example on a build bot: http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/11299

I was originally going to suggest a more verbose suffix much as Hal suggested, since any single letter is kind of meaningless. But to be honest, I hate the added verbosity and feel that instruction mnemonics are just an inherently tricky thing that requires experience with the ISA document.

Yeah, I actually added my initial comment precisely because I thought the existing form is more familiar to those that do not have deep experience with the ISA. What I mean is that the r is fairly intuitive to those of us that are deeply familiar with the ISA - it means "record-form". But to those less familiar with the ISA, the phrase "record-form" may be completely foreign and the choice of r would then be quite confusing. Whereas looking at the mnemonics in the ISA:

I am going to mark this as requiring changes until we can get more consensus on the direction. This is a very pervasive change that is completely subjective and as such should require agreement from more than two people that work on the target.

Hi Florian, this appears to break some of our downstream builds. Here's a minimal test case:

This looked really cool so I thought I'd try it on PPC. I hit the unreachable for an unknown op: <unknown op DW_OP_regx (144)>unknown op.

Thanks for cleaning this up. This seems completely obvious and non-controversial to me. Feel free to commit once the dependent patch lands.

In rG9353c5dd0664ea444236e527bf93566e11dc34df#855593, @xbolva00 wrote:

Disabled -Werror on sanitizer bots.

This changeset is a rather complicated and hard-to-follow piece of code that appears to solve a problem in the motivating test case, but makes it rather unclear whether it is a good idea or a bad idea overall.
There are a number of things that need to be clarified for this patch to proceed:

1. Needs empirical performance data. This is straightforward - see if it affects the performance of any benchmarks.
2. Needs more thorough testing. We need to cover more types, more ways we may end up with these "merge-and-store" idioms, different numbers of elements changed, etc.
3. An overview of the algorithm should be provided to aid readability. The code as written does not exactly aid readability so it would be good to provide an outline.
4. I still think we should do this in InstCombine rather than on the SDAG. It seems like that would be a more natural place to do this.
5. If we are doing it in InstCombine, why not simply produce a masked store? If we converted the IR for the test case in the description to this:

define dso_local void @foo(<4 x float>* nocapture %a, float %b) local_unnamed_addr #0 {
entry:
  call void @llvm.masked.store.v4f32.p0v4f32(<4 x float> <float 1.000000e+00, float undef, float undef, float 2.000000e+00>, <4 x float>* %a, i32 1, <4 x i1> <i1 true, i1 false, i1 false, i1 true>)
  ret void
}
declare void @llvm.masked.store.v4f32.p0v4f32(<4 x float>, <4 x float>*, i32, <4 x i1>)

We will get the desired codegen:

lis 4, 16256
lis 5, 16384
stw 4, 0(3)
stw 5, 12(3)

And there is target-independent handling for masked stores, so it is not at all clear to me why we'd go through the trouble of implementing this complex handling in the SDAG.

Added Eli and Justin in case they are interested in chiming in on at least the target-independent parts of this.

LGTM with some clarification in a comment.

LGTM.

LGTM.

LGTM. Thank you.

This needs to be a property of PPCFunctionInfo and set when the option is provided.

In D70466#1753176, @steven.zhang wrote:

The feature list here assume that, new processor will contain superset of features of those that came before it. And we are trying to break the assumption here. Can we do it another way. That is, keep the TD unchanged and check the CPU Directive in PPCSubtarget ? i.e.

PPCSubtarget::vectorsUseTwoUnits() const { return VectorsUseTwoUnits && DarwinDirective == PPC::DIR_PWR9; }

I need to add some additional pieces to this.

LGTM. Thanks for adding the test case.

LGTM except for formatting nits. Thanks for addressing the comments.

Rebased with the current source tree.
Updated the new test cases.
Improved some of the handling for the target pattern.
Refactored some of the original combines as the new combine overrode what they do.
Fixed some issue in the combine for load/convert/splat which were exposed by the new combine.

Thanks for your comments. I'll address them and commit this.

In D69497#1730212, @vddvss wrote:

Also, I noticed in the case PPC::XXPERMDI: block there are a couple of nested ifs without elses and checks on ifs that could simplified by converted to early breaks, albeit at the cost of polluting git blame with whitespace differences. Let me know if you are interested in a separate patch to clean some of that up.

LGTM. Thank you for your patience.