This is an archive of the discontinued LLVM Phabricator instance.

[GuardWidening] Use logical and in widenCondCommon as it stated in doc
AbandonedPublic

Authored by skatkov on Jun 21 2022, 10:54 PM.

Details

Summary

During the guard widening we can add a poison use in the first guard which
can result in undefined behavior which were not before due to this guard could
guards us from it.

To protect us we could use logical and instead of arithmetic and as it states in
widenCondCommon.

This is alternative solution to https://reviews.llvm.org/D128155.
What worries me is that usage of select may break parseRangeChecks logic
where it expects arithmetic and to split already widened condition...

Diff Detail

Event Timeline

skatkov created this revision.Jun 21 2022, 10:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 10:54 PM
skatkov requested review of this revision.Jun 21 2022, 10:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 10:54 PM
nikic added a comment.Jun 22 2022, 4:01 AM

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.

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.

Let's wait a bit what other say. But it may work.

Hm, this adds quite a bit of complexity.

A couple high level thoughts here:

  • A good chunk of that complexity comes from the fact that if the original condition involves an and this code expands out to the component checks and then rewrites all at once. Arguable, we should be able able to emit as LogicalAnd(Cond1, Cond2), and then optimize later, but that's not the way this code is currently structured. It might be worth thinking about whether we can reorder the optimization step. (Random thought, not a "please go do this first".)
  • The matching logic in this code should probably be extended to handle LogicalAnd.
  • Poison can only be introduced in the rewritten condition if a new value reaches the branch. The range check comparison itself can not produce poison. Given that, you may be able to simplify this as "identify values from Cond1 which already produce poison", and then "use logical and or freeze only on values in new range checks not in said set".
skatkov updated this revision to Diff 439301.Jun 23 2022, 2:55 AM

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

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

Next steps:

  1. Re-factor to simplify code.
  2. Add a lot of comments
  3. Carefully verify all tests to find bugs.
  4. Add freeze logic under an option.
skatkov updated this revision to Diff 439698.Jun 24 2022, 4:28 AM

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.

It would be good if someone could verify our conclusion. It looks like the area is very fragile.

nikic added inline comments.Jun 24 2022, 5:13 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
641

m_LogicalAnd also matches m_And, so you can drop the code above.

skatkov added inline comments.Jun 24 2022, 10:44 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
641

Thanks, will update on Monday.

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.

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.

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.

Abandon due to it is wrong way to fix this.

skatkov abandoned this revision.Jul 20 2022, 8:15 PM