On Mon, 25 Apr 2016, Uros Bizjak wrote:
> On Mon, Apr 25, 2016 at 4:47 PM, H.J. Lu <[email protected]> wrote:
> > On Mon, Apr 25, 2016 at 7:18 AM, Uros Bizjak <[email protected]> wrote:
> >> On Mon, Apr 25, 2016 at 2:51 PM, H.J. Lu <[email protected]> wrote:
> >>> Tested on Linux/x86-64. OK for trunk?
> >>
> >>> + /* FIXME: Since the CSE pass may change dominance info, which isn't
> >>> + expected by the fwprop pass, call free_dominance_info to
> >>> + invalidate dominance info. Otherwise, the fwprop pass may crash
> >>> + when dominance info is changed. */
> >>> + if (TARGET_64BIT)
> >>> + free_dominance_info (CDI_DOMINATORS);
> >>> +
> >>
> >> Please resolve the above problem first, target-dependent sources are
> >> not the place to apply band-aids for middle-end problems. The thread
> >> with the proposed fix died in [1].
> >>
> >> [1] https://gcc.gnu.org/ml/gcc/2016-03/msg00143.html
> >
> > free_dominance_info (CDI_DOMINATORS) has been called in other
> > places to avoid this middle-end issue. I don't know when the middle-end
> > will be fixed. I don't think this target optimization should be penalized
> > by
> > the middle-end issue.
>
> Let's ask Richard if he is OK with the workaround...
Well, it's ultimately your call (it's a workaround in the target).
Of course I'd like to see the underlying issue fixed and the
workarounds in "other places" be removed.
Richard.
> One more thing:
>
> @@ -3551,6 +3874,7 @@ convert_scalars_to_vector ()
> basic_block bb;
> bitmap candidates;
> int converted_insns = 0;
> + rtx zero, minus_one;
>
> bitmap_obstack_initialize (NULL);
> candidates = BITMAP_ALLOC (NULL);
> @@ -3585,22 +3909,40 @@ convert_scalars_to_vector ()
> if (dump_file)
> fprintf (dump_file, "There are no candidates for optimization.\n");
>
> + if (TARGET_64BIT)
> + {
> + zero = gen_reg_rtx (V1TImode);
> + minus_one = gen_reg_rtx (V1TImode);
> + }
> + else
> + {
> + zero = NULL_RTX;
> + minus_one = NULL_RTX;
> + }
> +
>
> Do we *really* need to crate registers here? They are only used as a
> temporary in scalar_chain_64::convert_insn, and I think that any CSE
> worth its name should find out when the same immediate is loaded to
> different temporaries.
>
> BTW: I'd really like if Ilya can review the functionality of the
> patch, he is an expert in this conversion stuff.
>
> Uros.
>
>
--
Richard Biener <[email protected]>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
