This got fixed already/identically with: 2c4c659

Adding more potential reviewers. I'm not sure if anyone is familiar with this code, but others definitely have a better understanding of the data structures than me.

Ignore the prior comment/update - I got the review numbers inverted. Sorry about that.

Updated diff to include clang tests that fail with the setting of the quiet bit.

Updated diff to include clang tests that fail with the setting of the quiet bit.

The clang test diffs got dropped from my diff...will update.

Patch updated:

1. Assert that losesInfo was set as expected if we lost the entire NaN payload.
2. Change code comment from 'TODO' to 'FIXME' based on discussion here.
3. Added APFloat unittests for better coverage of losesInfo and status.

In D87835#2291094, @efriedma wrote:

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

In D87835#2290688, @rjmccall wrote:

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

LGTM - this makes it more likely that we propagate as expected and cleans up the current code.
We probably have more cases like D88063, but that can be handled as follow-up patches.

LGTM

What, if any, interaction is there between this and D87361?
Can we solve this with the more general flag propagation scheme proposed there?

LGTM - thanks. But please do use clang-format to make the bots happy.

LGTM

Patch updated:

1. Use the direct approach to fix the bug - replace with undef before trying to delete.
2. Avoid branch on undef in crashing test case.

In D87149#2282484, @nikic wrote:

Actually, that would be out-of-bounds for instcombine. Removing a phi operand would mean removing the incoming block too, and we don't do that in instcombine. So I think we just bail out if a value still has uses. SimplifyCFG will eventually get it.

Replacing with undef before erasing would do as well.

Patch updated:
Updated code comments; no code or test changes.

In D87149#2282328, @spatel wrote:

I guess we could try to eliminate phi operands if an incoming block (g.exit) is unreachable? But simplifycfg should do that already, so I don't know if we want instcombine trying to do that too.

In D87149#2282232, @lebedev.ri wrote:

Unreachable BB's were the obvious potential source for non-empty use-count, but i thought instcombine didn't combine instructions in unreachable blocks.
Maybe that is the bug?

In D87149#2281145, @uabelho wrote:

Hi!

We've run into a case where instcombine crashes with this patch:

opt -o /dev/null bbi-47401.ll -instcombine

on

@c = external global i16*, align 1

define void @main() {
entry:
  br label %for.cond

for.cond:                                         ; preds = %g.exit, %entry
  %conv1 = phi double [ %conv, %g.exit ], [ undef, %entry ]
  br i1 undef, label %for.end, label %for.body

for.body:                                         ; preds = %for.cond
  %0 = load i16*, i16** @c, align 1
  %1 = load i16, i16* %0, align 1
  %conv = sitofp i16 %1 to double
  unreachable

g.exit:                                           ; No predecessors!
  br label %for.cond

for.end:                                          ; preds = %for.cond
  %conv1.lcssa = phi double [ %conv1, %for.cond ]
  store double %conv1.lcssa, double* undef, align 1
  ret void
}

results in

opt: ../lib/Transforms/InstCombine/InstCombineInternal.h:448: virtual llvm::Instruction *llvm::InstCombinerImpl::eraseInstFromFunction(llvm::Instruction &): Assertion `I.use_empty() && "Cannot erase instruction that is used!"' failed.

I suppose it's the dead block %g.exit that messes up things.

In D87188#2281957, @sanwou01 wrote:

I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned?

Patch updated:

1. Set the 2nd bit of the significand to preserve NaN-ness and signalling.
2. Added TODO comments for potentially better/different solutions.
3. Adjusted tests to provide better coverage of current behavior.

In D87835#2279926, @rjmccall wrote:

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

In D86160#2279646, @kazu wrote:

I've filed an issue for a performance regression caused by this patch:

https://bugs.llvm.org/show_bug.cgi?id=47558

LGTM - I think we should give this a try as-is (with the one-use check still there), see if anything regresses, then ease/remove the use check as a follow-on.
As noted, we may need to adjust cost models to account for the size/speed difference that's showing up in unrolling/inlining. That's probably because we assume that an intrinsic is expanded to a single instruction vs. the current cmp+sub+select being 3 instructions?

Patch updated:
Use stable_sort to reduce movement (not sure how to expose a difference in a test though).

LGTM

Patch updated:
Added a test to show forming a better reduction than we used to create.

In D86395#2274522, @Jac1494 wrote:

In D86395#2269436, @spatel wrote:

In D86395#2268634, @Jac1494 wrote:

test53 and test55 has been removed.

We are going in circles. I'll give this 1 more try, and if it still doesn't make sense, then maybe another reviewer can better communicate what we expect for the tests:

1. There are 4 commutative patterns variations in the code.
2. Each one of those patterns should have a test (we should have at least 4 tests).
3. Each test corresponds to exactly one of the commutative patterns.
4. Each test should be in canonical form without this patch (the baseline CHECK lines should correspond exactly to the IR as written).
5. The 8 step check I provided earlier may be used to confirm that the tests provide the expected coverage for the code.

@spatel, apologies for the confusion created due to multiple revisions.

Let's consider the initial pattern and associated commutative version of this.

initial pattern:-
(~A & B) ^ A -> (A | B) 

And 8 possible commutative version of this.
1) (~A & B) ^ A -> (A | B) 
2) (~B & A) ^ B -> (A | B)
3) (B & ~A) ^ A -> (A | B)  --> identical to 1)
4) A ^ (~A & B) -> (A | B)  --> identical to 1)
5) A ^ (B & ~A) -> (A | B)  --> identical to 1)
6) (A & ~B) ^ B -> (A | B)  --> identical to 2)
7) B ^ (~B & A) -> (A | B)  --> identical to 2)
8) B ^ (A & ~B) -> (A | B)  --> identical to 2)

As you may notice that all these patterns (IR) are reducing to 2 unique patterns (pattern 1 and pattern 2).
So pattern 1 and 2 are sufficient for the coverage of all commutative variations of the primary pattern.
Does this matches with you understandings ??

I have no experience with the sanitizer requirements, but IIUC we can't accurately sanitize any code that uses this transform with this restriction.
If that's what is happening/intended in other passes, I guess there's no other way around it.
But if it happening in other passes, shouldn't we use some standard function to bail out? For example llvm::mustSuppressSpeculation()?

Expanded patch still LGTM.
Side note: change "isLargest" name to "isLargestMagnitude" to make that API less confusing?

LGTM. Should we common/integrate the caller functions/switch cases? Ie, we're switching on min/max opcode, but then translating that knowledge into the bool flags and then predicating on those flags instead of the opcodes.

In D87555#2270191, @spatel wrote:

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have:

/// We can largely ignore the normal safety and cost constraints on speculation
/// of these ops when we find a matching pair. This is because we are already
/// guaranteed that any exceptions and most cost are already incurred by the
/// first member of the pair.

In other words, this is similar to hoisting a normal math binop once we find the other member of the pair.

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have: