[PING] genattrab.c generate switch

On Fri, 4 Mar 2016, David Malcolm wrote:

> On Thu, 2016-03-03 at 17:36 -0500, Patrick Palka wrote:
>> On Thu, Mar 3, 2016 at 4:29 PM, Jesper Broge Jørgensen
>> <jesperbroge@gmail.com> wrote:
>>>
>>> On 18/02/16 13:22, Bernd Schmidt wrote:
>>>>
>>>> On 01/19/2016 12:47 PM, Jesper Broge Jørgensen wrote:
>>>>>
>>>>> Here is the reformatted patch:
>>>>
>>>>
>>>> This will probably have to wait until stage1.
>>>>
>>>>> +      const int code = GET_CODE (op2);
>>>>> +      if (code != IOR)
>>>>> +    {
>>>>> +      if (code == EQ_ATTR)
>>>>
>>>>
>>>> All the formatting still looks completely mangled. This was
>>>> probably done
>>>> by your mailer. Please try attaching the diff as text/plain.
>>>>
>>>>
>>>> Bernd
>>>>
>>> Hi i send the patch back as an attatchment as requested about two
>>> weeks ago
>>> (https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01256.html) but i
>>> have not
>>> received any response.
>>>
>>> If it has to wait for stage 1 are there anything else i can do for
>>> the patch
>>> untill then?
>>
>> I still suggest to try making write_test_expr() avoid emitting
>> redundant parentheses for chains of || or &&, which would fix the
>> original issue all the same.  Previously you claimed that such a
>> change would not be simpler than your current patch, but I gave it a
>> quick try and ended up with a much smaller patch:
>>
>>  gcc/genattrtab.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> Patrick, did you forget to attach the patch?  I see the diffstat, but
> no patch.
>

Here it is:

-- >8 --

Subject: [PATCH] Reduce nesting of parentheses in conditionals generated by
  genattrtab

gcc/ChangeLog:

 	* genattrtab.c (write_test_expr): New parameter EMIT_PARENS
 	which defaults to true.  Emit an outer pair of parentheses only if
 	EMIT_PARENS.  When continuing a chain of && or ||, don't emit
 	parentheses for the right-hand operand.
---
  gcc/genattrtab.c | 26 +++++++++++++++++++-------
  1 file changed, 19 insertions(+), 7 deletions(-)

On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>> I still suggest to try making write_test_expr() avoid emitting
>>> redundant parentheses for chains of || or &&, which would fix the
>>> original issue all the same.  Previously you claimed that such a
>>> change would not be simpler than your current patch, but I gave it a
>>> quick try and ended up with a much smaller patch:

This looks like a reasonable stopgap if a release manager thinks this is 
important enough to fix for gcc-6. Some comments below for that case. 
Longer term I'm not sure - in theory maybe the switch would allow us to 
generate better code, but I tried it and code size actually seems to go 
up (could be jump tables however). I also noticed that in the version 
with the switch we still have cases of

switch (cached_type)
{
  ....
  default:
    switch (cached_type)
    {
    }
}

so that might be a point where the patch could be improved to see if we 
can get better code generation by collapsing these switches. Might also 
be worth trying to optimize this pattern in gcc.

>   static unsigned int
> -write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int
> flags)
> +write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int
> flags,
> +         bool emit_parens = true)

Indentation looks wrong, but could be mailer damage. You should also 
update the function comment.

> +      attrs_cached
> +        = write_test_expr (outf, XEXP (exp, 1), attrs_cached, flags,
> +                   need_parens);

Same here, watch indentation.


Bernd

On Fri, Mar 04, 2016 at 04:13:41PM +0100, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
> >>>I still suggest to try making write_test_expr() avoid emitting
> >>>redundant parentheses for chains of || or &&, which would fix the
> >>>original issue all the same.  Previously you claimed that such a
> >>>change would not be simpler than your current patch, but I gave it a
> >>>quick try and ended up with a much smaller patch:
> 
> This looks like a reasonable stopgap if a release manager thinks this is
> important enough to fix for gcc-6. Some comments below for that case. Longer

I think it is important for gcc-6, because one compiler for
weirdo reasons imposes unreasonably small restrictions on the () nesting
and some people use it as stage1 compiler.

So, with the formatting nits fixed, if it got appropriately tested, I think
we want it for stage4.

	Jakub

On 04/03/16 16:13, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>>> I still suggest to try making write_test_expr() avoid emitting
>>>> redundant parentheses for chains of || or &&, which would fix the
>>>> original issue all the same.  Previously you claimed that such a
>>>> change would not be simpler than your current patch, but I gave it a
>>>> quick try and ended up with a much smaller patch:
>
> This looks like a reasonable stopgap if a release manager thinks this 
> is important enough to fix for gcc-6. Some comments below for that 
> case. Longer term I'm not sure - in theory maybe the switch would 
> allow us to generate better code, but I tried it and code size 
> actually seems to go up (could be jump tables however). I also noticed 
> that in the version with the switch we still have cases of
>
> switch (cached_type)
> {
>  ....
>  default:
>    switch (cached_type)
>    {
>    }
> }
>
> so that might be a point where the patch could be improved to see if 
> we can get better code generation by collapsing these switches. Might 
> also be worth trying to optimize this pattern in gcc.
>
>
>
> Bernd
I can look into that if you deem it worth it, or would you rather just 
go with Patriks suppressed parenthesis?

On 03/04/2016 05:03 PM, Jesper Broge Jørgensen wrote:

> I can look into that if you deem it worth it, or would you rather just
> go with Patriks suppressed parenthesis?

For the moment (in stage4) we'll at most go with Patrick's patch. 
Whether we do anything beyond that depends on whether we can demonstrate 
a benefit - it might be worth investigating. Maybe more interesting than 
fixing genattrtab would be an optimization to identify switch patterns 
like the one I posted.


Bernd

On 03/04/2016 08:13 AM, Bernd Schmidt wrote:
> On 03/04/2016 03:27 PM, Patrick Palka wrote:
>>>> I still suggest to try making write_test_expr() avoid emitting
>>>> redundant parentheses for chains of || or &&, which would fix the
>>>> original issue all the same.  Previously you claimed that such a
>>>> change would not be simpler than your current patch, but I gave it a
>>>> quick try and ended up with a much smaller patch:
>
> This looks like a reasonable stopgap if a release manager thinks this is
> important enough to fix for gcc-6. Some comments below for that case.
> Longer term I'm not sure - in theory maybe the switch would allow us to
> generate better code, but I tried it and code size actually seems to go
> up (could be jump tables however). I also noticed that in the version
> with the switch we still have cases of
>
> switch (cached_type)
> {
>   ....
>   default:
>     switch (cached_type)
>     {
>     }
> }
>
> so that might be a point where the patch could be improved to see if we
> can get better code generation by collapsing these switches. Might also
> be worth trying to optimize this pattern in gcc.
There's probably a good number of things we could do for this and 
related scenarios.  Essentially DOM and threading are unlikely to do 
anything with this because the path from the outer case into the inner 
switch is going to have multiple values associated with cached_type.

The backwards threader, if it was presented with SSI rather than SSA 
could probably untangle it, but I'm not sure we're ready to make the 
jump to SSI (though it would simplify VRP & DOM).

Jeff