Not sure how to make Phab select the previous version of the patch.
It's showing the patch from rL335433 now, which is not what we want of course.
Did D48508 affect any of the tests or motivation for this one?

rL335433 is the commit for this review.

Oops - I included the wrong link in the commit message for rL335433; that was meant to correspond to D48508.

Patch updated - no functional change intended from the previous rev (test diffs are identical):

1. Refactored to reduce code duplication and improve readability.
2. Replaced getValueSizeInBits() == 1 with getValueType() == MVT::i1.

I was working on a generic DAGCombiner patch for what I think are the early patterns in the motivating case:
https://rise4fun.com/Alive/Gsyp

In D48467#1140891, @eraman wrote:

No, this doesn't deal with this. Should I start from the negate and push it
down if it is transitively used by fma?

Does this patch deal with the case if we negate the scalar before doing the splat?

Patch updated:
In the last rev, I forgot to remove the use of the original opcode when we create the new binop.
So we could have shifts when we should have multiplies (and the tests showed that).

Oops - just noticed a typo that makes this patch wrong.

Patch updated:
Committed the helper function change with rL335242, so this is only the shuffle fold now.

LGTM.

Adding potential reviewers based on a grep of 'deterministic' in recent commits messages. :)

Patch updated:
No functional change from the previous rev, but use early exits to minimize indents and add TODO comments for the planned enhancements.

LGTM.

In D48085#1136910, @lebedev.ri wrote:

In D48085#1136900, @spatel wrote:

In D48085#1136860, @lebedev.ri wrote:

Should this adjust the ReleaseNotes?

The docs provide extra warning/workaround, and this change doesn't affect that language IMO (there's less chance we're going to break code, but we don't have to disclose that).

For reference, here are links to the docs that we added with the previous patches (last updated with D46236 I think):
https://clang.llvm.org/docs/ReleaseNotes.html#new-compiler-flags
https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
http://llvm.org/docs/ReleaseNotes.html#non-comprehensive-list-of-changes-in-this-release

Let me know if you see anything that can be improved.

The text in first and last links suggests that such an optimization is always being done.

In D48085#1136910, @lebedev.ri wrote:

In D48085#1136900, @spatel wrote:

In D48085#1136860, @lebedev.ri wrote:

Should this adjust the ReleaseNotes?

The docs provide extra warning/workaround, and this change doesn't affect that language IMO (there's less chance we're going to break code, but we don't have to disclose that).

For reference, here are links to the docs that we added with the previous patches (last updated with D46236 I think):
https://clang.llvm.org/docs/ReleaseNotes.html#new-compiler-flags
https://clang.llvm.org/docs/UsersManual.html#controlling-code-generation
http://llvm.org/docs/ReleaseNotes.html#non-comprehensive-list-of-changes-in-this-release

Let me know if you see anything that can be improved.

The text in first and last links suggests that such an optimization is always being done.
But now it will only be done in presence of no-signed-zeros-fp-math attribute.
Which is controlled by -ffast-math (or maybe some more fine-grained option, too?)

Patch updated:
Added a 'TODO' comment about using a FABS-based sequence if we can't ignore -0.0.

In D48085#1136860, @lebedev.ri wrote:

Should this adjust the ReleaseNotes?

Ping.

Patch updated:

1. Improved header comment examples (use 'undef' rather than -1).
2. Reimplemented logic to maximize code reuse (make use of isSingleSourceMask() internally).
3. Added 'TODO' notes about allowing size-changing shuffles.

Do you have commit privileges?

LGTM - see inline for a nit.

I see the motivation better now, but I'm still worried about using ValueTracking.

I'm not sure how to test that potential late constant creation problem (see rL334977), but this change looks safe and does what the comment says, so LGTM.

On 2nd thought, I confused myself. :)
This patch is potentially dangerous - although it's hard to find cases where that danger propagates.
As with the earlier undef matcher changes, we have to audit the users to make sure they are not incorrectly using the matcher to propagate a value with undefs where it's not safe.

In D47983#1134797, @lebedev.ri wrote:

Ping.
Or is it intentional for m_APInt not to do this?

In D48023#1135195, @RKSimon wrote:

@spatel Should this wait for D48236 or can I commit it now?

Uploaded the wrong draft - went overboard on the 'const &' on that last one. An ArrayRef is a const ref.

Patch updated:
Redefine all of the Constant* functions in terms of ArrayRef<int> parent implementations. This makes the code slightly cleaner and might open up the possibility of removing some redundant code from the backend.

Patch updated:

1. Fixed undef checks to be "x == -1" rather than "x < 0".
2. Added assertions for inputs and logic.
3. Added more unit tests (the last group is based on the mask examples that I added to the header file code comments...would be extra embarrassing to get those wrong!).
4. Reduced confusing comment for transpose (and restated the canonical TRN1 / TRN2 examples for clarity).
5. Changed all 'unsigned' types to 'int' so we don't have to cast (this is always an annoyance with mask values).
6. Changed variable names and added locals to try to make the logic clearer and more uniform.

LGTM.

LGTM too. Thanks for finding the bug and working through the steps to make good patches.

LGTM.

LGTM.

Adding more potential FMA reviewers. There may be some tricky bits here in the interaction between the FMF, the global settings, and the TLI hooks. See for example D28675.

This reads clearer to me by avoiding the potential to confuse with UB. Should the same blurb apply to nsz with -0.0?

Might want to get a 2nd opinion on 512 though since I'm not as familiar with that.

In D48134#1131626, @rsmith wrote:

Can we mark these as argmemonly?