- User Since
- Jan 25 2017, 1:38 AM (284 w, 1 d)
Fri, Jul 1
I come up with this version of the solution.
Abandon due to it is wrong way to fix this.
Wed, Jun 29
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.
Tue, Jun 28
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.
Fri, Jun 24
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.
Thu, Jun 23
Step forward trying to take into account Philip's comments.
- Add LagicAnd parsing in range check parser.
- Utilize the isGuaranteedNotToBeUndefOrPoison function.
Wed, Jun 22
Update pat h before landing to handle reviewer's comment.
Tue, Jun 21
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.
Mon, Jun 20
Sun, Jun 19
Fri, Jun 17
Tue, Jun 14
Mon, Jun 13
Update test before landing.
Thu, Jun 9
Wed, Jun 8
Handled comments about tests.
Can you please land the test first and in this patch upload the diff what changed.
Jun 2 2022
ok, I take a liberty to land this and ready to revert or follow-up basing on @arsenm feedback.
May 30 2022
Thanks! I'll wait for 1-2 days for @arsenm feedback before landing the patch.
remove redundant asserts, add usage of defBB.
May 29 2022
May 27 2022
May 25 2022
May 14 2022
Apr 26 2022
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...
Apr 25 2022
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?
Apr 22 2022
Generally looks good. Could please land test first in the current implementation to see the difference caused by patch.
Apr 18 2022
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.