On 10/30/24 6:33 AM, Jovan Vukic wrote:
Thanks for the feedback on the first version of the patch. Accordingly:
I have corrected the code formatting as requested. I added new tests to
the existing file phi-opt-11.c, instead of creating a new one.
I performed testing before and after applying the patch on the x86
architecture, and I confirm that there are no new regressions.
The logic and general code of the patch itself have not been changed.
So the A EQ/NE B expression, we can reverse A and B in the expression
and still get the same result. But don't we have to be more careful for
the TRUE/FALSE arms of the ternary? For BIT_AND we need ? a : b for
BIT_IOR we need ? b : a.
I don't see that gets verified in the existing code or after your
change. I suspect I'm just missing something here. Can you clarify how
we verify that BIT_AND gets ? a : b for the true/false arms and that
BIT_IOR gets ? b : a for the true/false arms?
I did not communicate this clearly last time, but the existing optimization
simplifies the expression "(cond & (a == b)) ? a : b" to the simpler "b".
Similarly, the expression "(cond & (a == b)) ? b : a" simplifies to "a".
Thus, the existing and my optimization perform the following
simplifications:
(cond & (a == b)) ? a : b -> b
(cond & (a == b)) ? b : a -> a
(cond | (a != b)) ? a : b -> a
(cond | (a != b)) ? b : a -> b
This is well understood. The key in my mind is that for AND we always
select the FALSE arm. For IOR we always select the TRUE arm.
For this reason, for BIT_AND_EXPR when we have A EQ B, it is sufficient to
confirm that one operand matches the true/false arm and the other matches
the false/true arm. In both cases, we simplify the expression to the third
operand of the ternary operation (i.e., OP0 ? OP1 : OP2 simplifies to OP2).
Right. No concerns there.
This is achieved in the value_replacement function after successfully
setting the value of *code within the rhs_is_fed_for_value_replacement
function to EQ_EXPR.
For BIT_IOR_EXPR, the same check is performed for A NE B, except now
*code remains NE_EXPR, and then value_replacement returns the second
operand (i.e., OP0 ? OP1 : OP2 simplifies to OP1).
This is the part I'm struggling a bit with. But I'm guessing you're
referring to this code:
/* For NE_EXPR, we want to build an assignment result = arg where
arg is the PHI argument associated with the true edge. For
EQ_EXPR we want the PHI argument associated with the false edge. */
e = (code == NE_EXPR ? true_edge : false_edge);
If I understand everything correctly your assertion is that we'll only
get here for AND/EQ_EXPR and IOR/NE_EXPR. There's no way to get here
for AND/NE_EXPR or IOR/EQ_EXPR?
Intuitively that's probably true since if it wasn't we'd likely already
be generating incorrect code for AND with NE_EXPR. But I'd like you to
confirm my basic understanding.
Thanks,
jeff
