On Tue, Sep 27, 2011 at 04:26:47PM -0300, Alexandre Oliva wrote:
> On Sep 20, 2011, Jakub Jelinek <ja...@redhat.com> wrote:
>
> > For NOTE_INSN_CALL_ARG_LOCATION, the locations aren't location lists, but
> > a single location at the point of the call. They are independent of
> > all other locations, so any kind of caching only decreases the chance
> > that a suitable location
>
> With the proposed patch, we cache full expressions, and we should have
> an expression if there's a location for the expression (save for
> expression depth limits).
>
> > is found (and as the numbers show, it decreases it a lot).
>
> It decreased because of a bug: many equivalence expressions that used to
> be only in the cselib equivalence lists were no longer used with the
> proposed patch. I have a fix to bring them back in, but see below.
Ok.
> >> > can't it be postponed to start of vt_emit* phase
>
> >> If my hunch is correct, no. My concern is precisely that the equivalent
> >> may cease to hold (say once we cross a val_reset within a loop), but
> >> we'll keep on relying on it.
>
> > After vt_initialize, all cselib locations should hold are just some
> > expressions containing VALUEs, constants, ENTRY_VALUEs and nothing else,
> > all REGs and MEMs are supposed to be flushed. Those are equivalences that
> > are always constant, they don't need any kind of resetting and they never
> > cease to hold. Say VALUE3 is always VALUE1 + VALUE2.
>
> I realize that. The problem is, I had observed nonsensical equivalences
> in dataflow_set loc lists such as (plus (value) (const_int)) in the
> location list of the value itself (and a nonzero constant). After much
> pondering, I've concluded that this was a symptom of the first round of
> dataflow analysis in the initial implementation of VTA, in which we used
> union rather than intersection semantics. This is no longer the case,
> and I've now convinced myself this can't happen any more, so we can rely
> on cselib equivalences, not just for expanding location expressions as
> we did before my patch (and will do again in a subsequent version of
> it), but also for dataflow set merging (the next algorithm I'm going to
> try to optimize).
Ok.
> >> > Can't you use ENUM_BITFIELD (onepart_enum) onepart : 8; instead?
>
> >> I don't think it will then be packed in the same word as n_var_parts.
>
> > enum A { B, C, D };
> > struct S { char a; enum A b : 8; char c; char d; };
> > int i = sizeof (struct S);
>
> > results in i = 4 for all the cc1 and cc1plus cross compilers on my box I've
> > tried, with various ABI options.
>
> Yeah, I'm pretty sure GCC will behave like that on nearly all ABIs, but
> that's not mandated by standards. Indeed, IIRC even enum bitfields
> aren't mandated by standards. That's why we have ENUM_BITFIELD, after
> all! Anyway, since ENUM_BITFIELD was supposed to address this very
> issue, I guess it just makes sense for me to go with it ;-)
While C doesn't have enum bitfields in the standard, C++ does, so it isn't
something very strange. Furthermore, nothing relies on it being packed up
in the struct, it is just an optimization, so if some compiler doesn't
support it or doesn't pack it well, nothing bad happens.
> > If it is just performance optimization, I'd say there should be
> > gcc_checking_assert (DECL_RTL_SET_P (dtemp));
> > before it to verify and make it obvious that you aren't expecting it to be
> > NULL.
>
> I'm adding DECL_RTL_KNOWN_SET, with a DECL_RTL_SET_P checking_assert,
> and using that. How's that?
Fine with me.
Jakub
