Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger]

On 09/20/16 18:12, Kyrill Tkachov wrote:
>
> On 20/09/16 16:30, Bernd Schmidt wrote:
>> On 09/20/2016 05:18 PM, Jeff Law wrote:
>>>> I assume HARD_FRAME_POINTER_REGNUM is never zero.
>>> It could be zero.  It's just a hard register number.  No target has the
>>> property that its hard frame pointer register is 0 though :-)
>>
>> git blame to the rescue. The current state comes from one of
>> tbsaunde's cleanup patches:
>>
>> > diff --git a/gcc/regrename.c b/gcc/regrename.c
>> index 174d3b5..e5248a5 100644
>> --- a/gcc/regrename.c
>> +++ b/gcc/regrename.c
>> @@ -442,12 +442,10 @@ rename_chains (void)
>>         continue;
>>
>>        if (fixed_regs[reg] || global_regs[reg]
>> -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
>> -         || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
>> -#else
>> -         || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
>> -#endif
>> -         )
>> +         || (!HARD_FRAME_POINTER_IS_FRAME_POINTER &&
>> frame_pointer_needed
>> +             && reg == HARD_FRAME_POINTER_REGNUM)
>> +         || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
>> +             && reg == FRAME_POINTER_REGNUM))
>>         continue;
>>
>>        COPY_HARD_REG_SET (this_unavailable, unavailable);
>>
>> Looks like it never got reviewed and was committed as preapproved.
>>
>> The #ifdef we had before looks odd too. Maybe this should just read
>>
>>  if (fixed_regs[reg] || global_regs[reg]
>>      || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))
>>

I think this breaks the symmetry with the statement above.

how about this (similar to what Jeff suggested):

Index: regrename.c
===================================================================
--- regrename.c (revision 240268)
+++ regrename.c (working copy)
@@ -464,8 +464,7 @@
    if (frame_pointer_needed)
      {
        add_to_hard_reg_set (&unavailable, Pmode, FRAME_POINTER_REGNUM);
-      if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
-       add_to_hard_reg_set (&unavailable, Pmode, 
HARD_FRAME_POINTER_REGNUM);
+      add_to_hard_reg_set (&unavailable, Pmode, HARD_FRAME_POINTER_REGNUM);
      }

    FOR_EACH_VEC_ELT (id_to_chain, i, this_head)
@@ -479,10 +478,9 @@
         continue;

        if (fixed_regs[reg] || global_regs[reg]
-         || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
-             && reg == HARD_FRAME_POINTER_REGNUM)
-         || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
-             && reg == FRAME_POINTER_REGNUM))
+         || (frame_pointer_needed
+             && (reg == HARD_FRAME_POINTER_REGNUM
+                 || reg == FRAME_POINTER_REGNUMi)))
         continue;

        COPY_HARD_REG_SET (this_unavailable, unavailable);


FRAME_POINTER_REGNUM is always special, and it
may be identical with HFP.  But it is not necessary
to use HARD_FRAME_POINTER_IS_FRAME_POINTER at all.

Bernd.

>
> I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf.
>
> Thanks,
> Kyrill
>
>>
>> Bernd
>