In D72870#1826619, @spatel wrote:

Thanks for cleaning this up!

The changes all look logically fine to me, but there are numerous formatting problems (80-cols, indentation, etc.).

Try running "clang-format" on the patch?

Thanks for cleaning this up!

LGTM

I don't know enough about jump-threading to approve, but if this is correct, then we should keep the new pass manager in sync with this change?

Adding potential FP reviewers for question of gcc- (potentially-buggy-) compatibility.

This follows the reasoning that we discussed in the earlier discussion, but after re-reading the gcc comment in particular, I'm wondering whether this is what we really want to do...
If FAST_MATH is purely here for compatibility with gcc, then should we mimic gcc behavior in setting that macro even if we think it's buggy?
Ie, when we translate these settings to LLVM's FMF, we can still override the -ffast-math flag by checking the -ffp-contract flag (if I'm seeing it correctly, the existing code will pass that alongside -ffast-math when contract is set to on/off).

In D72861#1824947, @nikic wrote:

@spatel I've reported https://bugs.llvm.org/show_bug.cgi?id=44573 and https://bugs.llvm.org/show_bug.cgi?id=44574 to keep track of this.

For the new PM question, I assume that's an oversight. I haven't looked too much into the new PM, but I have noticed other possible mistakes there. Eg, it seems like that's running the SpeculativeExecutionPass on all targets?
https://github.com/llvm/llvm-project/blob/53b68e676faf208b4a8f817e9bd4ddd522cc6006/llvm/lib/Passes/PassBuilder.cpp#L427

In D72675#1824659, @wristow wrote:

This all sounds good to me.

So to make sure we're all on the same page, my understanding is that the plan forward is:

1. Make the Clang change first (including adding another pair of tests for -ffp-contract=on).
2. Update the LLVM tests illustrating the current baseline state.
3. Make the LLVM change shown here, and update the tests with the fixes.
4. Bring in the DAG combiner work that Michael has done internally at Apple.
   
   Points 1, 2, and 3 are essentially the points in the patch posted here, so I'll do that. And of course Michael will then take on point 4.
   
   Is that the plan? If yes, I'll transition this item to just be the Clang pieces, and I'll spin off a new one to do the LLVM portion of what is posted here.

In D72675#1823227, @mcberg2017 wrote:

We crossed that bridge internally at Apple a while ago, meaning I have some code debt for cleaning up open source for fma formation that uses contract and reassoc differently than we do today, both together and separately, case by case.

Patch updated:
Bail out on anything not integer or FP. I'm not sure how to deal with pointers, but added a few basic tests in case we do eventually support those.

I would've taken the test down to 5 or 10 to make it easier to see without scrolling, but this is fine. LGTM.

In D72807#1822794, @nikic wrote:

I've added the complete original test case with -instcombine-infinite-loop-threshold=2. It would also be possible to reduce it (it just wouldn't need the full 1000 iterations that trigger the default threshold), but as the test now runs quickly even on a debug build, I figured keeping the original is fine.

In D72733#1820606, @lebedev.ri wrote:

I'm not sure what kind of fallout this will cause.
For example, won't this affect 'saturating math' peephole?
Won't this affect clamp (min-max) pattern?

I don't know anything about experimental.guard either, but the change LGTM based on similar transforms.
Instcombine's visit* contract is:

// Visitation implementation - Implement instruction combining for different
// instruction types.  The semantics are as follows:
// Return Value:
//    null        - No change was made
//     I          - Change was made, I is still valid, I may be dead though
//   otherwise    - Change was made, replace I with returned instruction

I wish we didn't have such complicated transforms in instcombine, but that's out-of-bounds for this patch. :)
LGTM

Patch updated:

1. Added code comments to better explain the strategy.
2. Moved calls lower and within predicate blocks (expect V2 to be undef) where this makes sense to try.

In D71568#1812498, @danlark wrote:

In D71568#1812463, @danlark wrote:

In D71568#1812426, @spatel wrote:

In D71568#1812355, @spatel wrote:

There's a lot going on here, and the patch doesn't apply cleanly to current source. Can you pre-commit any NFC changes/tests to make this patch smaller? For example, the udiv change/tests are not affected by the mul code?

Sorry - I didn't see earlier that this patch is part of a sequence.
I'm a bit skeptical about the need to extend the "udiv action" machine. Are the motivating cases really that complicated or could we get away with a simpler pattern match of mul(select...)?

I am not sure but I just reused the code for both division and multiplication -- from my perspective the idea is the same.

Also, I rebased a bit and made the patch smaller as you asked.

So, I basically don't see any reason division machine is doing a lot of things but as abstractions and semantics are mostly the same, I decided to reuse it. And to generalize power 2 multiplication then.

That's true that most of patterns are just one mul(select) but I saw at least one where the depth level was two.

This change would also remove the logical gymnastics needed to give special meaning to the mask with respect to undef/poison:
http://llvm.org/docs/LangRef.html#shufflevector-instruction

LGTM

In D71568#1812355, @spatel wrote:

There's a lot going on here, and the patch doesn't apply cleanly to current source. Can you pre-commit any NFC changes/tests to make this patch smaller? For example, the udiv change/tests are not affected by the mul code?

There's a lot going on here, and the patch doesn't apply cleanly to current source. Can you pre-commit any NFC changes/tests to make this patch smaller? For example, the udiv change/tests are not affected by the mul code?

LGTM

LGTM

In D72361#1810410, @lebedev.ri wrote:

In D72361#1810243, @spatel wrote:

Patch updated:
Made predicate more restrictive and added asserts.

Can a test case be constructed for that?

(That case could be implemented as a shuffle, if we don't do that already)

I'm very surprised we didn't hit this sooner, but we have this set of 'select' folds in InstCombiner::visitSelectInst() that have existed for at least 10 years, and they are all poison-unsafe:

Thanks all for the reductions!

Patch updated:
Made predicate more restrictive and added asserts.

Patch updated:
Rebased after pre-committing the cleanup with rG780ba1f22b53.

LGTM - I'll try to push this on behalf of the author shortly.

In D72361#1809619, @RKSimon wrote:

LGTM - some of NFC refactoring as pre-commits would make sense to separate it from the new features.

In D72300#1807837, @RKSimon wrote:

Its fine to leave it where it is - LGTM cheers.

In D72300#1807578, @RKSimon wrote:

Why doesn't partitionShuffleOfConcats catch this?

I think this is safe now (although limited to a single basic block), but I don't have enough experience with memop transforms to confidently approve. Please get a 2nd look from @lebedev.ri @jdoerfert @efriedma or someone else.

Patch updated:
Only do the transform if both arms of the select are cmps. Defer canonicalization of single cmp pattern (see TODO comments).

In D72007#1804065, @lebedev.ri wrote:

Looks good to me in principle, the case where we get rid of not is good,
but i'm not sure about the case where we only manage to sink it into one of the hands.
To me that seems to be the opposite of the current general canonicalizaion we do.