This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR36988 - work around our bogus handling of whether a stmt is volatile or not
On Fri, 1 Aug 2008, Diego Novillo wrote:
> On Fri, Aug 1, 2008 at 06:00, Richard Guenther <rguenther@suse.de> wrote:
> >
> > Sigh. These things never stop.
> >
> > typedef struct {
> > unsigned char mbxCommand;
> > } MAILBOX_t;
> > void lpfc_sli_brdrestart(void)
> > {
> > volatile unsigned int word0;
> > MAILBOX_t *mb;
> > mb = (MAILBOX_t *) &word0;
> > mb->mbxCommand = 0x1A;
> > __writel((*(unsigned int *) mb));
> > }
> >
> > So, we CCP &word0 into *mb.0_2 in
> >
> > <bb 2>:
> > mb_1 = (struct MAILBOX_t *) &word0;
> > mb_1->mbxCommand ={v} 26;
> > mb.0_2 = (unsigned int *) mb_1;
> > D.1953_3 = *mb.0_2;
> >
> > which is ok, but since word0 is volatile (and thus has TREE_SIDE_EFFECTS
> > set)
> > the assert in gimple_rhs_has_side_effects complains about the statement
> > not being volatile, which is bogus.
>
> Are you referring to the 'mb.0_2 = (unsigned int *) mb_1'
> statement? If so, I agree, that statement should not have
> volatile operands because &word0 is not volatile.
>
> Propagation, however, will convert 'D.1953_3 = *mb.0_2' into a
> volatile load.
Yes. Even if this is conservatively correct it is not what the
original program does. Now, we have no way to distinguish these here ...
> > The real problem here is that we do not trust the statement state
> > (this wasn't a volatile load in the source, so it should not become one
> > by mere propagation either), but try to verify it using state on the
> > operands.
>
> I disagree. Propagation can make a pointer dereference into a
> volatile load. Like the assignment to D.1953_3 above.
Sure - it even _must_ in the above case, because we have no choice
to preserve the original program behavior. Whether a load is volatile
or not doesn't depend on the variable we load from but on the
semantics of the initial IL. Consider
int i;
volatile int *p = &i;
*p;
propagation would turn the volatile load into a non-volatile load
which would be a wrong transformation. Now, we have no way to
say the load is volatile for
i;
as we cannot simply set TREE_SIDE_EFFECTS on is VAR_DECL.
Instead we should have marked the statement and simply keep such
annotation throughout transformations.
>
> > Now, this "plugs another hole" regarding to the issue above. But in
> > the end after gimplification we should never need to look at
> > TREE_SIDE_EFFECTS or TREE_THIS_VOLATILE -- the semantics are fixed
> > by the frontend and should be transitioned to a proper place by
> > the gimplifer (that gimple_has_volatile_ops is re-computed all the
> > time by the operand scanner doesn't help here of course, neither
> > that it is abused by the we-don't-have-alias-information code during
> > early optimization ...)
>
> What do you think would be a better way of handling pointer
> dereferences before alias analysis? We currently set
> volatile_ops to true so that optimizers won't touch those
> statements. We could use a different marker, but then optimizers
> would have to check that too.
stmt_references_memory looks better to me. gimple_stmt_has_volatile_ops
can based on gimple_aliases_computed return true for such.
> > Of course there's not space for two more bits on a gimple statement
> > to say does_volatile_load and does_volatile_store (appearantly some
> > predicates want to treat them differently).
>
> I can't parse this.
See above. I want a way to say that
d.123 = *p;
is a volatile load and stick this information on the statement. Then
I can simply propagate i and the statement will still be volatile.
Richard.