I do not see any visible regressions: https://llvm-compile-time-tracker.com/compare.php?from=692344d87357ded619d216b265a9375f4326d8fb&to=a701b7e368b70688bb4b84dafcaa43fa7c9a3649&stat=instructions:u

Thank you, Eli.

Thank you, Eli.

Hi Eli, as no other feedbacks for a while, may I ask you to review the patch (it looks like it is a simple dead code elimination) and approve if there is no objections?

In D158503#4607337, @efriedma wrote:

AArch64 changes look fine; it looks like atomic lowering is generating weird code that gets simplified here. (It was mostly getting simplified anyway, but after register allocation, so the register couldn't be eliminated.)

ping.

Thanks.

I wonder whether it will easier to review if I split this patch into three one:

1. Use wouldInstructionBeTriviallyDead instead of checking side-effect and terminator only to extend the scope of collecting ephemeral values
2. Use CodeMetrics utility in CodeGenPrepare
3. Extend the algorithm for collecting ephemeral values by checking cycles (induction variable case)

Looks reasonable.

ping, any feedback?

One comment, other looks good...

In D152358#4456534, @jdoerfert wrote:

We have ephemeral value trackers and other helpers, we should use those, assuming this is going forward.

I've taken a look at CodeMetrics::collectEphemeralValues. It does not recognize loops with phi node as well. I can extend its detector, however it collects ephemeral values basing on conditions "I->mayHaveSideEffects() || I->isTerminator()".

In the commit message adds a couple of words about the reason - IRCE expects that true branch comes to loop.

LGTM with nit.

You can pre-land tests without review.

Any words in git commit message?

In D152358#4456534, @jdoerfert wrote:

We have ephemeral value trackers and other helpers, we should use those, assuming this is going forward.

In D152358#4454950, @nikic wrote:

Our longer term plan was to stop removing assumes in CGP, so they remain available during codegen, in particular for AA purposes. Instead, we would drop assumes more aggressively earlier if they keep control flow alive.

It seems pretty likely that assumes that keep alive otherwise dead IVs are also blocking optimizations in earlier parts of the pipeline, and we would be better of dropping such assumes earlier. I'm not entirely sure what the right heuristic for that would be, but "ephemeral values of the assume include a phi node" is probably a pretty good one.

Friendly ping...

Comment update.

one more function renamed to start with lower symbol.

please, take a look.

Thanks a lot for review. I'll update a patch as soon as we agreed on terminology (Closure) and description of FindClosureOfWouldBeTriviallyDeadInstructions function.

Test are updated. Code re-factored. Anything I can do to land this?

is it better?

ping

Fix comments.

In D151482#4387272, @dmgreen wrote:

I think that adding a new reduction intrinsic sounds like a good addition. We should even have native instructions we could use for it under AArch64. For Arm we have seemed to have never added support for llvm.minimum/maximum. (There isn't a good instruction for it, and there isn't any generic expansion, but adding a new type of reduction shouldn't make that any worse).

In D151482#4385109, @anna wrote:

Looking at X86 lowering for the vector reduce intrinsics, we do not handle signed zeroes. So, we will need to introduce new intrinsics as llvm.vector.reduce.fminimum and llvm.vector.reduce.fmaximum, which is similar to existing vector reduce intrinsics but handles signed zeroes and NaN.

LGTM with comment for consideration - I do not insist.

In D151082#4363081, @apilipenko wrote:

Why do we even care that an edge from a widenable condition branch terminates with a deopt? Widenable condition alone should be enough for widening.

We might want to know that a widened condition will result in a deopt for profitability purposes. We know that a deopt path is very unlikely and the runtime will be able to heal from the widening if we go that path. But we can check for these concerns separately and explicitly. We can use BFI to determine if a path is rare. We can introduce a new metadata to mark branches that can be healed after widening.

BTW, we use make implicit metadata for a similar purpose - to indicate that a branch can be "speculated" and the runtime can heal from this speculation.

At the moment I do not see simple way to implement https://reviews.llvm.org/D148514, so I put this on hold due to temporary busy with other stuff.

At the moment, I do not see an easy way to implement this one in platform independent way. I put it on hold due to busy with other stuff.

LGTM, @nikic, any remaining concerns?

PTAL

I've pre-landed the new tests, fixed the name in the test and updated this revision.

In D149844#4320910, @e-kud wrote:

Maybe a few more vector tests? Existing tests cover the no-nans and no-signed-zeros case, they were added to avoid bloat of scalarization. E.g. a test with all checks (generic vectorized version) and two test with constant vectors to eliminate a zero check and a NaN check.

Handled comments.

I wonder whether any problems with landing this patch?

And actually instead of NFC tag you can just say [TEST]

In D148001#4297996, @mkazantsev wrote:

In D148001#4295213, @skatkov wrote:

General question: don't you want to separate sub and add handling in completely different functions.
To be honest, there will be small code duplication but in this case no need to mixup add and sub logic.
It will be easier to review and support in the future.

If duplication is a problem you can create functions for sub and add to keep first and last lines are in common.

But code for sub is exactly same as the code that handles both add and sub, with 2 lines of difference.

In D145634#4295568, @e-kud wrote:

In D145634#4294388, @skatkov wrote:

In D145634#4294383, @skatkov wrote:

In D145634#4294264, @e-kud wrote:

In D145634#4291019, @skatkov wrote:

I have a side question (not delaying landing this one)

It looks like this change lowers vectorized form of intrinsic in a non-optimal way.
So I wonder whether there are some plans to improve it as follow-up?

Yes, for sure. I've tried to find a way to implement vectorized version for SSE, but nothing's come to my mind. We need at least SSE2 for PCMPEQ{W,D} because all fp comparison instructions treat -0.0 and 0.0 as equal. It seems that pentium3 will suffer from not optimal fmaximum/fminimum...

Why we cannot do something like this https://godbolt.org/z/Yxfn3jTj1 ?
I mean at least for avx?

BTW. my measurements on micro benchmarking (I know micro might cause that branch predictor works good), the version https://godbolt.org/z/rEj9GPfnY is the best one for scalar but it cannot be implemented in SelectionDAG as has a CFG.

Both versions are incorrect. They don't work as expected in case of (-0.0, 0.0), (0.0, -0.0) inputs. Because comiss and max treat negative and positive zeros as equal.

General question: don't you want to separate sub and add handling in completely different functions.
To be honest, there will be small code duplication but in this case no need to mixup add and sub logic.
It will be easier to review and support in the future.