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

[Andrew MacLeod]

On 11/13/2014 09:34 AM, Richard Biener wrote:
> 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.
>
>
Ah, so it does.  Nevermind :-)

Andrew