Message ID | CA+=Sn1k0kJaSERkK8LZF7=Nt-avZ7fwRydKod4t8uYpPmC06HA@mail.gmail.com |
---|---|

State | New |

Headers | show |

Andrew Pinski <andrew.pinski@caviumnetworks.com> writes: > Hi, > For some reason cse produced: > (insn 27 43 28 2 (set (reg:SI 231 [+4 ]) > (plus:SI (reg:SI 229 [+4 ]) > (const_int 0 [0]))) t.c:24 10 {*addsi3} > (nil)) > > (insn 28 27 29 2 (set (reg:SI 212) > (ltu:SI (reg:SI 231 [+4 ]) > (reg:SI 229 [+4 ]))) t.c:24 521 {*sltu_sisi} > (nil)) > > And then forwprop goes and does the following: > In insn 28, replacing > (ltu:SI (reg:SI 231 [+4 ]) > (reg:SI 229 [+4 ])) > with (const_int 1 [0x1]) > > This is due to simplify-rtx.c (simplify_relational_operation_1) Having > the following optimization: > /* (LTU/GEU (PLUS a C) C), where C is constant, can be simplified to > (GEU/LTU a -C). Likewise for (LTU/GEU (PLUS a C) a). */ > > But this is wrong when C is 0. Anyways I committed this patch as > obvious to fix the issue in simplify-rtx.c but I have not looked into > why CSE does simplify the plus. Hmm, like you said originally in the PR, I think generating the invalid PLUS is the bug here. I agree (as Richard B said) that an invalid PLUS shouldn't cause us to generate wrong code, but an assert seems better than a check. Eric, what do you think? Richard > PR rtl-opt/54524 > * simplify-rtx.c (simplify_relational_operation_1): Don't simplify > (LTU/GEU (PLUS a 0) 0) into (GEU/LTU a 0) since they are not equivalent. > > Index: simplify-rtx.c > =================================================================== > --- simplify-rtx.c (revision 193110) > +++ simplify-rtx.c (working copy) > @@ -4546,7 +4546,9 @@ simplify_relational_operation_1 (enum rt > && GET_CODE (op0) == PLUS > && CONST_INT_P (XEXP (op0, 1)) > && (rtx_equal_p (op1, XEXP (op0, 0)) > - || rtx_equal_p (op1, XEXP (op0, 1)))) > + || rtx_equal_p (op1, XEXP (op0, 1))) > + /* (LTU/GEU (PLUS a 0) 0) is not the same as (GEU/LTU a 0). */ > + && XEXP (op0, 1) != const0_rtx) > { > rtx new_cmp > = simplify_gen_unary (NEG, cmp_mode, XEXP (op0, 1), cmp_mode);

> Hmm, like you said originally in the PR, I think generating the invalid > PLUS is the bug here. I agree (as Richard B said) that an invalid PLUS > shouldn't cause us to generate wrong code, but an assert seems better > than a check. Eric, what do you think? I think that this boils down to deciding whether (PLUS X 0) is invalid or merely non-canonical. If the former, then I agree that an assertion is appropriate; if the latter, then I don't think that we should stop the compiler because of it. No strong opinion, although I'd lean towards the latter. In any case, the piece of code which generates this PLUS needs also to be fixed (at least on the mainline).

Index: simplify-rtx.c =================================================================== --- simplify-rtx.c (revision 193110) +++ simplify-rtx.c (working copy) @@ -4546,7 +4546,9 @@ simplify_relational_operation_1 (enum rt && GET_CODE (op0) == PLUS && CONST_INT_P (XEXP (op0, 1)) && (rtx_equal_p (op1, XEXP (op0, 0)) - || rtx_equal_p (op1, XEXP (op0, 1)))) + || rtx_equal_p (op1, XEXP (op0, 1))) + /* (LTU/GEU (PLUS a 0) 0) is not the same as (GEU/LTU a 0). */ + && XEXP (op0, 1) != const0_rtx) { rtx new_cmp = simplify_gen_unary (NEG, cmp_mode, XEXP (op0, 1), cmp_mode);