In D59909#1544824, @RKSimon wrote:

@spatel @andreadb Any comments?

In D63390#1546687, @lebedev.ri wrote:

Thank you for the reviews.

In D63390#1546676, @spatel wrote:

LGTM. I have no idea if it's worth the trade-off, but we could do this sooner in IR (instcombine) instead of or in addition to SDAG?

Indeed, this fold is missed in middle-end too, but the regression at hand is in back-end test,
so i'm not sure if we should just hand-wave and only fix it in middle-end.
Also, where should the middle-end fix be? Again InstCombine?

This matches my mental model for FMF propagation, so LGTM.

LGTM. I have no idea if it's worth the trade-off, but we could do this sooner in IR (instcombine) instead of or in addition to SDAG?

In D63405#1546424, @arsenm wrote:

In D63405#1546410, @spatel wrote:

In D63405#1546348, @arsenm wrote:

In D63405#1546342, @spatel wrote:

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

I would expect the lowering here to just preserve the flag on the original instruction, and not propagate the source (as was done here previously). The corresponding expansion in the DAG has a TODO to preserve the flags, so there isn't precedent for this

Let me know if I'm not seeing this correctly (haven't looked at MIR very much - it might help if the test was committed with the baseline output first, so we just have a diff in this patch).
Did we start with 'fneg arcp X' and end up with 'fsub arcp nsz -0.0, X'? Preserving (rather than unioning) the flag should end up with only 'fsub arcp -0.0, X'?

The pre-patch code would fold fneg (fadd flags x, y) -> fsub flags -0.0, (fadd flags x, y), and entirely ignore any flags on fneg. I'm not entirely sure this is correct on its own

In D63405#1546348, @arsenm wrote:

In D63405#1546342, @spatel wrote:

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

I would expect the lowering here to just preserve the flag on the original instruction, and not propagate the source (as was done here previously). The corresponding expansion in the DAG has a TODO to preserve the flags, so there isn't precedent for this

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

In D63364#1544585, @lebedev.ri wrote:

Looks ok.
Is there some costmodel here, or do we always (well, when we see concatenation, we don't seem to introduce it
intentionally) want to do this, in the hope that two smaller ops are always at least as good as one wider op?

Patch updated with test changes:

1. Added a bigger test for the original PR37428 example because that shows a difference: with AVX1, we do not split the ymm ops any more. I'm not sure if that's better or worse than having mul/sitofp ops in the loop, but since the original request is for an SSE target, I think we can deal with that independently.
2. Added an "enable-debugify" RUN to the IR test file to verify that we are not creating naked instructions. The IRBuilder takes care of this for us here, but I fixed a related problem in rL363409.

For reference, the vector shift TLI hook was originally added for a related CGP transform:
rL201655

LGTM - but let's wait a ~day to commit in case anyone else has comments.

We have 2 text comment acknowledgements ("Seems fine", "Looks ok") - thanks for the prompt reviews!...can someone make it official by toggling the Phab state to 'Approved'? :)

Patch updated:

1. Add text to header comment to indicate that this may be more powerful than getSplatValue().
2. Add TODO comment about recursing through several other types of operands.

Ping * 2.

This matches the existing fold with 2 fsubs, and I agree with the comment in the description regarding IEEE-754 - fsub doesn't need to preserve the sign bit of a NaN. LGTM.

LGTM

In D62629#1536402, @cameron.mcinally wrote:

Check hasOneUse() before performing combine.

In D63038#1535140, @xbolva00 wrote:

In D63038#1535121, @nikic wrote:

Can you please provide some more information under which circumstances powf(x, (float) y) will provide a different result than powi(x, y), and which fast-math flags specifically are necessary to make that transform legal?

I don’t have such info, I personally think this is fine also without fast math, but since I cannot say it for sure, In the first version, I put it under fast math.

Maybe some experts like @spatel or @efriedma can confirm that this transformation is always fine?

LGTM

LGTM

In D63004#1535595, @xbolva00 wrote:

@spatel Thanks for review

What about current revision? Is it OK for you?

In D63004#1534341, @xbolva00 wrote:

@spatel Maybe we should just expand in it instcombine? I found something interesting...

In D62713#1535547, @spatel wrote:

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

LGTM

Patch updated:
Only add a check for isKnownNeverNaN(). We can remove FMF.noNaNs() independently when that's less likely to introduce regressions.

How does the output differ from the default expansion/optimization of ctpop? Please commit the tests with baseline CHECK lines, so we just show that diff. Add tests for another target too (AArch64?).