I come up with this version of the solution.

Abandon due to it is wrong way to fix this.

In D128322#3617923, @nikic wrote:

In D128322#3617593, @skatkov wrote:

Dear reviewers, after deeper investigation the issue I guess I come up with the tests https://reviews.llvm.org/D128779 showing the logical and does not resolve the issue and correctly used freeze the only choice.
I would very appreciate if you could review the tests and confirm that my understanding is valid.

Yes, I agree with your analysis. The @simple_case test is clearest one I think, because it hoists a condition across other control flow.

Basically, if you have two adjacent llvm.experimental.guard (or separated by must-exec instructions), then they can be combined with logical and. But if they're separated by any (normal or abnormal) control flow, then the second condition must be frozen.

So yeah, we should go back to your original solution using freeze. Sorry for the red herring about logical and.

No need for sorry. It helped to go deeper into optimization. Simple freeze for combine checks does not work as well. Need to figure out how to deal with that.

Landed.

handle nits.

Add test with side effect for combine range checks.

Dear reviewers, after deeper investigation the issue I guess I come up with the tests https://reviews.llvm.org/D128779 showing the logical and does not resolve the issue and correctly used freeze the only choice.
I would very appreciate if you could review the tests and confirm that my understanding is valid.

After long offline conversation with Max, we came to conclusion that combineCheck case does not require logical and.
It based on the facts that all conditions in this case are nuw/nsw if first condition is true and the first condition nuw/nsw flags cannot be based on other conditions (due to its constant is minimal one) and can be safely hoisted before the guard as its poison flags (if they exist) are based on something else then on other conditions.

Step forward trying to take into account Philip's comments.

1. Add LagicAnd parsing in range check parser.
2. Utilize the isGuaranteedNotToBeUndefOrPoison function.

Update pat h before landing to handle reviewer's comment.

In D128322#3601261, @nikic wrote:

I don't follow the implementation details here, but based on test changes this looks like the right approach.

What worries me is that usage of select may break parseRangeChecks logic
where it expects arithmetic and to split already widened condition...

Possibly you want to replace the matcher there with m_LogicalAnd. This is safe if the range checks in question are of the form X pred C1 && X pred C2, i.e. both work on the same variable, and the other side is a constant. If not, then more care might be needed.

lgtm.

Please take a look the alternative solution with logical and: https://reviews.llvm.org/D128322

Update test before landing.

The test was obtained by bugpoint. I'll do my best to reduce it more.

I tend to re-write the patch and introduce the option to use freeze or logical and then choose the best one.
To have improved version of this patch for freeze and implement select solution I'll need to do some preparation work to know what condition comes from the widening guard (it should be first in logcal and and there is no need to freeze it) and what comes from other guard.
So, please be tuned.

ping

In D128155#3596981, @nikic wrote:

Does this actually need freeze, rather than just logical and instead of bitwise and?

ping

lgtm

Comments updated.

In D126523#3580887, @sameerds wrote:

Looks straightforward enough to me. If the value is a PHI, it must be at the beginning of the current block. If it is outside this block, then whoever registered it is confident that it dominates this block. Is it correct that the SSAUpdater will not end up in a state where it tries to use a phi to produce another phi in the same block? Because that might race against the assumption that the used phi is available earlier in the block.

ping.

Update test before landing.

Thanks!

Handled comments about tests.

Can you please land the test first and in this patch upload the diff what changed.

ping

ok, I take a liberty to land this and ready to revert or follow-up basing on @arsenm feedback.

Handled comments

Thanks! I'll wait for 1-2 days for @arsenm feedback before landing the patch.

remove redundant asserts, add usage of defBB.

In D126525#3544896, @sameerds wrote:

In D126525#3544805, @skatkov wrote:

In D126525#3542391, @djtodoro wrote:

This is interesting! Can you please make a simple/dedicated LLVM IR case for this only, so we can go over the example to get the whole picture?

llvm/test/CodeGen/AArch64/tail-dup-redundant-phi.mir is dedicated test for this patch and https://reviews.llvm.org/D126523. This test is not landed yet. I just uploaded it here as diff to show the difference.

Even then, a target independent test in LLVM IR will certainly be useful.

In D126525#3542391, @djtodoro wrote:

This is interesting! Can you please make a simple/dedicated LLVM IR case for this only, so we can go over the example to get the whole picture?

In D126354#3536815, @mkazantsev wrote:

Looks good, but please regenerate checks here. It's not clear at all what this test is trying to ensure (e.g. what is wide.chk).

In D126354#3536967, @nikic wrote:

Not familiar with this pass, but I suspect the right fix for this might be to use CreateLogicalAnd instead of CreateAnd.

Good catch.

And thanks for the discussion anyway - the topic is pretty complex and discuss it with someone is really helpful.

Sequential consistency adds total order of all seq_cst operations to release-acquire semantic. However If load is not used its total order does not matter, so only acquire semantic plays role here.

Please go through all comments in findBasePointer and verify that they are up to date with you changes...

Well.. Do I understand correctly that the original problem comes from the fact that during lowering of statepoint if value is relocated through register then this register is exported unconditionally?

Thanks, for you comment. Let me add details about my reasoning.
Let's we have load atomic %a with acquire or seq_cst semantic and this load has no uses.
According to specification this load has a ordering semantic only if it observes the value stored with release semantic to the same memory location.
If there is no uses then explicit check for is impossible. But it can be a implicit check like in example you mentioned.
So to do this implicit check we should load a value before our load and check its value. If our load must have the same value as previous load the previously loaded value may be used for such check.
Side note, let's consider there is only one store release to avoid mentioning "observe release store or later store" all the time.

Hi Eli, nice example. Do I understand correctly that here is the problem with exactly using of monotonic? If we used unordered (aka relaxed) then load2 can be safely removed?

In D124247#3468179, @efriedma wrote:

This isn't trivially correct.

For example, say you have a sequence "%load1 = load seq_cst %a; %load2 = load relaxed %a". I think the later load can sort of "inherit" the sequential consistency of the earlier load; the values it can contain are restricted. (I mean, maybe I'm missing something, but it's not as simple as "no one will check what actually value has been read".)

Generally looks good. Could please land test first in the current implementation to see the difference caused by patch.

I'm also looking into this. The idea is clear but I worry this adds a complexity with decoding the state.
I'm looking into the way to make it more straightforward. Probably it will require some re-factoring.