Clean up null check handling in EffectAnalyzer#8422
Conversation
Each visitor previously handled implicit null checks of reference operands in an ad hoc way. Different expressions had varying levels of precision when reasoning about the null checks. For example, many would just always set `implicitTraps`, while others would look at the nullability of the operands to be more precise. Factor the handling of null checks into a helper function and use it everywhere applicable. This makes the code smaller, clearer, and more precise. Update OptimizeCasts to add a new dummy operand to a dummy expression because EffectAnalyzer will newly try to look at that operand and it must not read uninitialized memory. Some GUFA tests are updated to remove more side-effect-free expressions now that EffectAnalyzer is more aggressive and can recognize them as such.
| assert(!expr || expr->type.isRef()); | ||
| if (expr && expr->type.isNull()) { | ||
| parent.trap = true; | ||
| return true; |
There was a problem hiding this comment.
IIUC, when we see a null, we mark it as trapping and tell the parent to stop computing effects. So e.g. a StructSet to a null would end up with the effects of { trap } when before this PR it had { trap, writes }.
We have several places where an analysis does .trap = false /* trapping is ok to ignore here, for reasons */ - after this PR, we would have no effects there at all. Is that still correct in those places?
There was a problem hiding this comment.
Yes, this should still be correct. The expression truly has no side effects other than to trap in such cases. Those passes must handle (unreachable) correctly, and this is no different.
There was a problem hiding this comment.
ok, I am still slightly worried... but we do have a fuzzer 😄
There was a problem hiding this comment.
Yes, I will fuzz this heavily before landing.
src/ir/effects.h
Outdated
|
|
||
| // Handle effects due to an explicit null check of the operands in `exprs`. | ||
| // Returns true iff there is no need to consider further effects. | ||
| bool handleNullChecked(std::initializer_list<Expression*> exprs) { |
There was a problem hiding this comment.
| bool handleNullChecked(std::initializer_list<Expression*> exprs) { | |
| bool handleTrapOnNull(std::initializer_list<Expression*> exprs) { |
Perhaps this is clearer?
There was a problem hiding this comment.
(since a "null check" is a more general term. also this is shorter)
src/ir/effects.h
Outdated
| if (trapOnNull(curr->ref)) { | ||
| return; | ||
| } | ||
| // OOB acces to array. |
There was a problem hiding this comment.
| // OOB acces to array. | |
| // OOB access to array. |
2 participants
Each visitor previously handled implicit null checks of reference
operands in an ad hoc way. Different expressions had varying levels of
precision when reasoning about the null checks. For example, many would
just always set
implicitTraps, while others would look at thenullability of the operands to be more precise.
Factor the handling of null checks into a helper function and use it
everywhere applicable. This makes the code smaller, clearer, and more
precise. Update OptimizeCasts to add a new dummy operand to a dummy
expression because EffectAnalyzer will newly try to look at that
operand and it must not read uninitialized memory.
Some GUFA tests are updated to remove more side-effect-free expressions
now that EffectAnalyzer is more aggressive and can recognize them as
such.