This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix the x86 *_doubleword_mask* patterns (PR target/85582)

On Wed, May 2, 2018 at 10:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, May 02, 2018 at 09:50:07AM +0200, Uros Bizjak wrote:
>> > 2018-05-02  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >         PR target/85582
>> >         * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
>> >         *ashl<dwi>3_doubleword_mask_1, *<shift_insn><dwi>3_doubleword_mask,
>> >         *<shift_insn><dwi>3_doubleword_mask_1): If and[sq]i3 is needed, don't
>> >         clobber operands[2], instead use a new pseudo.  Formatting fixes.
>> >
>> >         * gcc.c-torture/execute/pr85582-1.c: New test.
>> >         * gcc.c-torture/execute/pr85582-2.c: New test.
>>
>> O.
>
> Thanks, committed.  BTW, thinking about these some more,
> isn't INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
> incorrect?  Say for <MODE_SIZE> being 4 this is INTVAL (operands[3]) <= 31
> so it will accept & 0 to & 31 (that is correct), or e.g. & -2 (that is
> incorrect).

Hm, I wasn't thinking about undefined cases here.

> Isn't the right guarding condition that
> (INTVAL (operands[3]) & (<MODE_SIZE> * BITS_PER_UNIT)) == 0
> i.e. that we have guarantee the shift count doesn't have the topmost
> relevant bit set and we can ignore bits above it (UB)?

Yes, based on above, it looks more robust, indeed.

> And for the decision if we should use a masking or not perhaps
> if ((INTVAL (operands[3]) & ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
>     != (<MODE_SIZE> * BITS_PER_UNIT) - 1)
> , again ignoring bits we don't care about.

Yes, looks more robust vs. UB, too (but updated guarding condition
won't allow UB values here). Let's also change this, for consistency.

The patch that changes the condition to your approach is pre-approved.

Thanks,
Uros.

>> > --- gcc/config/i386/i386.md.jj  2018-05-01 12:18:15.566832552 +0200
>> > +++ gcc/config/i386/i386.md     2018-05-01 13:09:36.635164943 +0200
>> > @@ -10366,7 +10366,7 @@ (define_insn_and_split "*ashl<dwi>3_doub
>> >               (match_operand:SI 2 "register_operand" "c")
>> >               (match_operand:SI 3 "const_int_operand")) 0)))
>> >     (clobber (reg:CC FLAGS_REG))]
>> > -  "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT)-1
>> > +  "INTVAL (operands[3]) <= (<MODE_SIZE> * BITS_PER_UNIT) - 1
>> >     && can_create_pseudo_p ()"
>> >    "#"
>> >    "&& 1"
>> > @@ -10385,8 +10385,12 @@ (define_insn_and_split "*ashl<dwi>3_doub
>> >
>> >    operands[8] = GEN_INT (<MODE_SIZE> * BITS_PER_UNIT);
>> >
>> > -  if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT)-1)
>> > -    emit_insn (gen_andsi3 (operands[2], operands[2], operands[3]));
>> > +  if (INTVAL (operands[3]) < (<MODE_SIZE> * BITS_PER_UNIT) - 1)
>> > +    {
>> > +      rtx tem = gen_reg_rtx (SImode);
>> > +      emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
>> > +      operands[2] = tem;
>> > +    }
>> >
>> >    operands[2] = gen_lowpart (QImode, operands[2]);
>> >
>
>         Jakub
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]