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, i386, Pointer Bounds Checker 31/x] Pointer Bounds Checker builtins for i386 target


2014-10-10 20:45 GMT+04:00 Jeff Law <law@redhat.com>:
> On 10/09/14 10:54, Uros Bizjak wrote:
>>
>> On Thu, Oct 9, 2014 at 4:07 PM, Ilya Enkovich <enkovich.gnu@gmail.com>
>> wrote:
>>>
>>> It appeared I changed a semantics of BNDMK expand when replaced tree
>>> operations with rtl ones.
>>>
>>> Original code:
>>>
>>> +      op1 = expand_normal (fold_build2 (PLUS_EXPR, TREE_TYPE (arg1),
>>> +                                       arg1, integer_minus_one_node));
>>> +      op1 = force_reg (Pmode, op1);
>>>
>>> Modified code:
>>>
>>> +      op1 = expand_normal (arg1);
>>> +
>>> +      if (!register_operand (op1, Pmode))
>>> +       op1 = ix86_zero_extend_to_Pmode (op1);
>>> +
>>> +      /* Builtin arg1 is size of block but instruction op1 should
>>> +        be (size - 1).  */
>>> +      op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
>>> +                                op1, 1, OPTAB_DIRECT);
>>>
>>> The problem is that in the fixed version we may modify value of a pseudo
>>> register into which arg1 is expanded which means incorrect value for all
>>> following usages of arg1.
>>>
>>> Didn't reveal it early because programs surprisingly rarely hit this bug.
>>> I do following change to fix it:
>>>
>>>         op1 = expand_simple_binop (Pmode, PLUS, op1, constm1_rtx,
>>> -                                op1, 1, OPTAB_DIRECT);
>>> +                                NULL, 1, OPTAB_DIRECT);
>>>
>>> Similar problem was also fixed for BNDNARROW.  Does it look OK?
>>
>>
>> I'm not aware of this type of limitation, and there are quite some
>> similar constructs in i386.c. It is hard to say without the testcase
>> what happens, perhaps one of RTX experts (CC'd) can advise what is
>> recommended here.
>
> The problem is the call to expand_simple_binop.
>
> The source (op1) and the destination (op1) are obviously the same, so its
> going to clobber whatever value is in there.  If there are other uses of the
> original value of op1, then things aren't going to work. But I'm a little
> unclear how there's be other later uses of that value.  Perhaps Ilya could
> comment on that.

op1 is a result of expand_normal called for SSA name.  Other uses of
op1 come from expand of uses of this SSA name in GIMPLE code.

>
> Regardless, unless there's a strong reason to do so, I'd generally recommend
> passing a NULL_RTX as the target for expansions so that you always get a new
> pseudo.  Lots of optimizers in the RTL world work better if we don't have
> pseudos with multiple assignments.  By passing NULL_RTX for the target we
> get that property more often.  So a change like Ilya suggests (though I'd
> use NULL_RTX rather than NULL) makes sense.

Will replace it with NULL_RTX.

Thanks,
Ilya

>
>
>
> Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]