[gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign

[Andrew MacLeod]

On 11/13/2014 05:45 AM, Richard Biener wrote:
> On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm <dmalcolm@redhat.com> wrote:
>> On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote:
>>> On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Mon, Nov 10, 2014 at 05:27:50PM -0500, David Malcolm wrote:
>>>>> On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
>>>>>> On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
>>>>>>> To be constructive here - the above case is from within a
>>>>>>> GIMPLE_ASSIGN case label
>>>>>>> and thus I'd have expected
>>>>>>>
>>>>>>>      case GIMPLE_ASSIGN:
>>>>>>>        {
>>>>>>>          gassign *a1 = as_a <gassign *> (s1);
>>>>>>>          gassign *a2 = as_a <gassign *> (s2);
>>>>>>>        lhs1 = gimple_assign_lhs (a1);
>>>>>>>        lhs2 = gimple_assign_lhs (a2);
>>>>>>>        if (TREE_CODE (lhs1) != SSA_NAME
>>>>>>>            && TREE_CODE (lhs2) != SSA_NAME)
>>>>>>>          return (operand_equal_p (lhs1, lhs2, 0)
>>>>>>>                  && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
>>>>>>>                                                   gimple_assign_rhs1 (a2)));
>>>>>>>        else if (TREE_CODE (lhs1) == SSA_NAME
>>>>>>>                 && TREE_CODE (lhs2) == SSA_NAME)
>>>>>>>          return vn_valueize (lhs1) == vn_valueize (lhs2);
>>>>>>>        return false;
>>>>>>>        }
>>>>>>>
>>>>>>> instead.  That's the kind of changes I have expected and have approved of.
>>>>>> But even that looks like just adding extra work for all developers, with no
>>>>>> gain.  You only have to add extra code and extra temporaries, in switches
>>>>>> typically also have to add {} because of the temporaries and thus extra
>>>>>> indentation level, and it doesn't simplify anything in the code.
>>>>> The branch attempts to use the C++ typesystem to capture information
>>>>> about the kinds of gimple statement we expect, both:
>>>>>    (A) so that the compiler can detect type errors, and
>>>>>    (B) as a comprehension aid to the human reader of the code
>>>>>
>>>>> The ideal here is when function params and struct field can be
>>>>> strengthened from "gimple" to a subclass ptr.  This captures the
>>>>> knowledge that every use of a function or within a struct has a given
>>>>> gimple code.
>>>> I just don't like all the as_a/is_a stuff enforced everywhere,
>>>> it means more typing, more temporaries, more indentation.
>>>> So, as I view it, instead of the checks being done cheaply (yes, I think
>>>> the gimple checking as we have right now is very cheap) under the
>>>> hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes
>>>> put the burden on the developers, who has to check that manually through
>>>> the as_a/is_a stuff everywhere, more typing and uglier syntax.
>>>> I just don't see that as a step forward, instead a huge step backwards.
>>>> But perhaps I'm alone with this.
>>>> Can you e.g. compare the size of - lines in your patchset combined, and
>>>> size of + lines in your patchset?  As in, if your changes lead to less
>>>> typing or more.
>>> I see two ways out here.  One is to add overloads to all the functions
>>> taking the special types like
>>>
>>> tree
>>> gimple_assign_rhs1 (gimple *);
>>>
>>> or simply add
>>>
>>> gassign *operator ()(gimple *g) { return as_a <gassign *> (g); }
>>>
>>> into a gimple-compat.h header which you include in places that
>>> are not converted "nicely".
>> Thanks for the suggestions.
>>
>> Am I missing something, or is the gimple-compat.h idea above not valid C
>> ++?
>>
>> Note that "gimple" is still a typedef to
>>    gimple_statement_base *
>> (as noted before, the gimple -> gimple * change would break everyone
>> else's patches, so we talked about that as a followup patch for early
>> stage3).
>>
>> Given that, if I try to create an "operator ()" outside of a class, I
>> get this error:
>>
>> ‘gassign* operator()(gimple)’ must be a nonstatic member function
>>
>> which is emitted from cp/decl.c's grok_op_properties:
>>        /* An operator function must either be a non-static member function
>>           or have at least one parameter of a class, a reference to a class,
>>           an enumeration, or a reference to an enumeration.  13.4.0.6 */
>>
>> I tried making it a member function of gimple_statement_base, but that
>> doesn't work either: we want a conversion
>>    from a gimple_statement_base * to a gassign *, not
>>    from a gimple_statement_base   to a gassign *.
>>
>> Is there some syntactic trick here that I'm missing?  Sorry if I'm being
>> dumb (I can imagine there's a way of doing it by making "gimple" become
>> some kind of wrapped ptr class, but that way lies madness, surely).
> Hmm.
>
> struct assign;
> struct base {
>    operator assign *() const { return (assign *)this; }
> };
> struct assign : base {
> };
>
> void foo (assign *);
> void bar (base *b)
> {
>    foo (b);
> }
>
> doesn't work, but
>
> void bar (base &b)
> {
>    foo (b);
> }
>
> does.  Indeed C++ doesn't seem to provide what is necessary
> for the compat trick :(
>
> So the gimple-compat.h header would need to provide
> additional overloads for the affected functions like
>
> inline tree
> gimple_assign_rhs1 (gimple *g)
> {
>    return gimple_assign_rhs1 (as_a <gassign *> (g));
> }
>
> that would work for me as well.
>
>
Won't that defeat the desire for checking tho?  If you dont do a 
dyn_cast<>  in gimple_assign_rhs1 (gimple *g)  anyone can call it and 
upcast any kind of gimple into a gassign without checking that it is 
really a gassign...   Actually, a gcc_assert here may be sufficient... 
and better.

It'd be pretty easy to miss someone calling it when its not a gassign.

Andrew