On Mon, 2013-11-04 at 16:43 -0500, David Malcolm wrote:
> On Mon, 2013-11-04 at 08:19 -0500, Andrew MacLeod wrote:
> > On 11/01/2013 06:58 PM, David Malcolm wrote:
> > > On Fri, 2013-11-01 at 22:57 +0100, Jakub Jelinek wrote:
> > >> On Fri, Nov 01, 2013 at 05:47:14PM -0400, Andrew MacLeod wrote:
> > >>> On 11/01/2013 05:41 PM, Jakub Jelinek wrote:
> > >>>> On Fri, Nov 01, 2013 at 05:36:34PM -0400, Andrew MacLeod wrote:
> > >>>>> static inline void
> > >>>>> ! gimple_call_set_lhs (gimple gs, tree lhs)
> > >>>>> {
> > >>>>> - GIMPLE_CHECK (gs, GIMPLE_CALL);
> > >> The checking you are removing here.
> > >>
> > >>> What checking? There ought to be no checking at all in this
> > >>> example... gimple_build_call_vec returns a gimple_call, and
> > >>> gimple_call_set_lhs() doesn't have to check anything because it
> > >>> only accepts gimple_call's.. so there is no checking other than the
> > >>> usual "does my parameter match" that the compiler has to do...
> > >> and want to replace it by checking of the types at compile time.
> > >> The problem is that it uglifies the source too much, and, when you
> > >> actually don't have a gimple_call but supposedly a base class of it,
> > >> I expect you'd do as_a which is not only further uglification, but has
> > >> runtime cost also for --enable-checking=release.
> > > I can have a look next week at every call to gimple_call_set_lhs in the
> > > tree, and see to what extent we know at compile-time that the initial
> > > arg is indeed a call (of the ones I quickly grepped just now, most are
> > > from gimple_build_call and friends, but one was from a gimple_copy).
> > >
> > > FWIW I did some performance testing of the is_a/as_a code in the earlier
> > > version of the patch, and it didn't have a noticable runtime cost
> > > compared to the GIMPLE_CHECK in the existing code:
> > > Size of compiler executable:
> > > http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01920.html
> > > Compile times:
> > > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00171.html
> > I actually really dislike as_a<> and is_a<>, and think code needs to be
> > restructured rather than use them, other than possibly at the very
> > bottom level when we're allocating memory or something like that, or
> > some kind of emergency :-)... If we require frequent uses of those,
> > I'd be against it, I find them quite ugly.
> >
> > Like I said in the other reply, no rush, I don't think any of this
> > follow up is appropriate this late in stage 1.
I wasn't aware that there was a ramp in conservatism within stage1 - I
thought that we had a "large (tested) changes are OK" attitude
throughout all of stage1, and that the switch to conservatism only began
at the transition to stage3. (and have been frantically attempting to
get my big changes in before November 21st - should I be rethinking
this?)
> It would be more of an
> > "interest" examination right now.. at least in my opinion... I suspect
> > thinks like gimple_assign are more complex cases, but without looking
> > its hard to tell for sure.
>
> I tried converting gimple_call_set_lhs to accept a gimple_call, rather
> than a gimple, and excitingly, it was easiest to also convert
> cgraph_edge's call_stmt to also be a gimple_call, rather than just a
> gimple.
>
> Am attaching a patch (on top of the patch series being discussed) which
> adds this compile-time typesafety; bootstrap is in-progress. IMHO very
> little use of is-a.h was needed (5 instances of as_a, and 3 of dyn_cast;
> no use of is_a).
>
> I'm also attaching a followup patch which eliminates gimple_call_set_lhs
> in favor of a method of gimple_statement_call.
(I posted the two patches in my prioer email more as a talking point
than for detailed review, sorry if that wasn't clear).
FWIW, my preferred strategy for 4.9 is a compromise: to do the 1st patch
but not the 2nd: i.e. to make the existing gimple_foo_bar accessor
functions be typesafe at compile-time, but without converting them to
C++ methods - I think this gives us compile-time type-safety, whilst
minimizing code-churn and minimizing the more "C++ish" aspects that may
alarm those less happy with C++ in the codebase.
I believe it would be easiest to do them one subclass at-a-time e.g.
convert all gimple_call_* functions to accepting a gimple_call, then
another subclass etc - assuming that this approach is blessed by the
core reviewers.
Dave
