On Fri, Mar 04, 2016 at 06:49:58PM +0100, Bernd Schmidt wrote:
> On 03/04/2016 06:27 PM, Bernd Schmidt wrote:
> >On 03/04/2016 06:14 PM, Patrick Palka wrote:
> >
> >>I just quickly tested building the generated insn-attrtab.c with and
> >>without the patch using my host gcc 5.3 compiler and the .s output is
> >>not the same.
> >
> >Hmm, looking at the 003t.original dump it looks like there are
> >differences in SAVE_EXPRs. Indeed we seem to generate different code for
> >
> >int at;
> >
> >int foo ()
> >{
> > if (at == 2 || at == 4 || at == 7)
> > return 1;
> > return 0;
> >}
> >
> >int bar ()
> >{
> > if (at == 2 || (at == 4 || at == 7))
> > return 1;
> > return 0;
> >}
>
> Ahh... it's not just different placement of SAVE_EXPRs, it's actually a case
> of TRUTH_ORIF_EXPR vs. TRUTH_OR_EXPR (the distinction is invisible in the
> dumps), the latter being created by fold_range_test. That's a bit of a
> broken optimization what with its inability to see more than two comparisons
> at a time... we convert one ORIF per function, but a different one.
I think we don't need to guarantee identical assembly, the reason I've
suggested that was if it passed, it would be much easier to verify.
Without that, I think it should be bootstrapped at least on one other
target. Note the cases you remove the parens aren't just || and &&, but
most likely also | and & (at least there is some flag whether to print those
as && or &). And there is code for the caching of the attributes where the
result is still usable, I believe the patch doesn't break that, but it
wouldn't hurt to verify that.
Jakub
