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

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Nov 13, 2014 at 3:24 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> 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.

as_a performs that checking.

Richard.

> Andrew
>
>