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, MPX, 2/X] Pointers Checker [7/25] Suppress BUILT_IN_CHKP_ARG_BND optimizations.


2013/11/6 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Nov 6, 2013 at 3:04 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>>> On Wed, Nov 6, 2013 at 2:31 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>>>>> On Wed, Nov 6, 2013 at 12:59 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>>>>>>> On Wed, Nov 6, 2013 at 12:00 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>> 2013/11/6 Richard Biener <richard.guenther@gmail.com>:
>>>>>>>>> On Tue, Nov 5, 2013 at 3:24 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>>>> 2013/11/5 Richard Biener <richard.guenther@gmail.com>:
>>>>>>>>>>> On Tue, Nov 5, 2013 at 1:52 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> For input parameter P I need to have
>>>>>>>>>>>>   BOUNDS = __builtin_arg_bnd (P)
>>>>>>>>>>>> to somehow refer to bounds of P in GIMPLE.  Optimizations may modify
>>>>>>>>>>>> __builtin_arg_bnd (P) replacing P with its copy or some value. It
>>>>>>>>>>>> makes call useless because removes information about parameter whose
>>>>>>>>>>>> bounds we refer to. I want such optimization to ignore
>>>>>>>>>>>> __builtin_arg_bnd calls and always leave default SSA_NAME of PARM_DECL
>>>>>>>>>>>> there as arg.
>>>>>>>>>>>
>>>>>>>>>>> How does a compilable testcase look like that shows how the default def
>>>>>>>>>>> is used?  And what transforms break that use?  I really cannot see
>>>>>>>>>>> how this would happen (and you didn't add a testcase).
>>>>>>>>>>
>>>>>>>>>> Here is a test source:
>>>>>>>>>>
>>>>>>>>>> extern int bar1 (int *p);
>>>>>>>>>> extern int bar2 (int);
>>>>>>>>>>
>>>>>>>>>> int foo (int *p)
>>>>>>>>>> {
>>>>>>>>>>   if (!p)
>>>>>>>>>>     return bar1 (p);
>>>>>>>>>>   return bar2 (10);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> After instrumentation GIMPLE looks like:
>>>>>>>>>>
>>>>>>>>>> foo (int * p)
>>>>>>>>>> {
>>>>>>>>>>   <unnamed type> __bound_tmp.0;
>>>>>>>>>>   int _1;
>>>>>>>>>>   int _6;
>>>>>>>>>>   int _8;
>>>>>>>>>>
>>>>>>>>>>   <bb 6>:
>>>>>>>>>>   __bound_tmp.0_9 = __builtin_ia32_arg_bnd (p_3(D));
>>>>>>>>>>
>>>>>>>>>>   <bb 2>:
>>>>>>>>>>   if (p_3(D) == 0B)
>>>>>>>>>>     goto <bb 3>;
>>>>>>>>>>   else
>>>>>>>>>>     goto <bb 4>;
>>>>>>>>>>
>>>>>>>>>>   <bb 3>:
>>>>>>>>>>   _6 = bar1 (p_3(D), __bound_tmp.0_9);
>>>>>>>>>>   goto <bb 5>;
>>>>>>>>>>
>>>>>>>>>>   <bb 4>:
>>>>>>>>>>   _8 = bar2 (10);
>>>>>>>>>>
>>>>>>>>>>   <bb 5>:
>>>>>>>>>>   # _1 = PHI <_6(3), _8(4)>
>>>>>>>>>>   return _1;
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Here is optimized GIMPLE (if I do not apply my changes in tree-ssa-dom.c):
>>>>>>>>>>
>>>>>>>>>> foo (int * p)
>>>>>>>>>> {
>>>>>>>>>>   <unnamed type> __bound_tmp.0;
>>>>>>>>>>   int _1;
>>>>>>>>>>   int _6;
>>>>>>>>>>   int _8;
>>>>>>>>>>
>>>>>>>>>>   <bb 2>:
>>>>>>>>>>   if (p_3(D) == 0B)
>>>>>>>>>>     goto <bb 3>;
>>>>>>>>>>   else
>>>>>>>>>>     goto <bb 4>;
>>>>>>>>>>
>>>>>>>>>>   <bb 3>:
>>>>>>>>>>   __bound_tmp.0_9 = __builtin_ia32_arg_bnd (0B); [return slot optimization]
>>>>>>>>>>   _6 = bar1 (0B, __bound_tmp.0_9); [tail call]
>>>>>>>>>>   goto <bb 5>;
>>>>>>>>>>
>>>>>>>>>>   <bb 4>:
>>>>>>>>>>   _8 = bar2 (10); [tail call]
>>>>>>>>>>
>>>>>>>>>>   <bb 5>:
>>>>>>>>>>   # _1 = PHI <_6(3), _8(4)>
>>>>>>>>>>   return _1;
>>>>>>>>>>
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Now during expand or inline of foo I cannot determine the value for
>>>>>>>>>> __bound_tmp.0_9.
>>>>>>>>>
>>>>>>>>> Well, but clearly you can see that bounds for p == NULL are
>>>>>>>>> useless and thus there is no problem with the transform.
>>>>>>>>
>>>>>>>> This example just demonstrates the issue. I may use some var's address
>>>>>>>> in comparison with the similar result. And value does not always allow
>>>>>>>> to determine bounds, because pointers with the same value may have
>>>>>>>> different bounds.
>>>>>>>
>>>>>>> Some vars address can be used for better bounds deriving though.
>>>>>>> Well, it clearly shows it's a "get me good results" issue and not
>>>>>>> a correctness issue.  And for "good results" you trade in missed
>>>>>>> general optimizations.  That doesn't sound very good to me.
>>>>>>
>>>>>> That is definitely a correctness issue.
>>>>>
>>>>> Then please add a runtime testcase that fails before and passes after
>>>>> your patch.
>>>>>
>>>>>> Compiler cannot assume bounds
>>>>>> by pointer value ignoring bounds passed to the function.
>>>>>
>>>>> But it can certainly drop bounds to "unknown"?  You certainly cannot
>>>>> rely on no such transform taking place.  Maybe this just shows that
>>>>> your "refering to the parameter" is done wrong and shouldn't have
>>>>> used the SSA name default def.  What do you do for parameters
>>>>> that have their address taken btw?  You'll get
>>>>>
>>>>> foo (void *p)
>>>>> {
>>>>>    p_2 = *p;
>>>>>    __builtin_ia32_arg_bnd (p_2);
>>>>>
>>>>> which isn't desired?
>>>>
>>>> In this case you just make a load and bounds for p_2 are obtained
>>>> using bndldx call. I would be glad to use better way to refer to the
>>>> argument but I do not see one.
>>>
>>> Err, my mistake - I meant
>>>
>>> foo (void *p)
>>> {
>>>   p_2 = p;
>>>   __builtin_ia32_arg_bnd (p_2);
>>
>> Well, it does not make much difference. Such params are allocated on
>> the stack by expand. So, here p_2 = p is load from memory and bounds
>> should be loaded by bndldx. Expand is responsible to store bounds
>> using bndstx when it allocates p on the stack.
>
> The parameter is incoming in a register still.

It does not matter. In GIMPLE it is still load. During expand
assign_parms allocates slot on the stack for it and stores reg to it.
DECL_RTL for PARM_DECL is set to stack slot.

I just added code to store corresponding bounds for stored param (I
did not send this patch yet).

Ilya
>
>>>
>>>>>
>>>>> Hmm, unfortunately on x86_64 I get
>>>>>
>>>>> ./cc1 -quiet t.c -fcheck-pointer-bounds
>>>>> t.c:1:0: error: -fcheck-pointers is not supported for this target
>>>>>  extern int bar1 (int *p);
>>>>>  ^
>>>>>
>>>>> trying a simple testcase.  Grepping shows me no target supports
>>>>> chkp_bound_mode ..?  What's this stuff in our tree that has no way
>>>>> of testing it ... :/
>>>>
>>>> You should pass -mmpx to enable it on the target. Current version in
>>>> the mpx tree should build most of tests correctly but it is not
>>>> updated to the current state.
>>>
>>>> ./cc1 -quiet t.c -fcheck-pointer-bounds -mmpx
>>> t.c:1:0: error: -fcheck-pointers is not supported for this target
>>>  extern int bar1 (int *p);
>>>  ^
>>>
>>> hmm, maybe it's configure disabled somehow (this is just my dev tree).
>>> But I still see no chkp_bound_mode implementation.
>>
>> I'll check it and probably update the branch.
>
> I'm looking at trunk which has loads of this stuff already.
>
>>>
>>> Note that POINTER_BOUNDS_TYPE shouldn't have been a new
>>> tree code either but should be a RECORD_TYPE.  See how
>>> va_list doesn't use a new TREE_CODE either.
>>
>> va_list is either a pointer or pointer to a record (array of one
>> record) and is handled as a regular pointer to a regular record.
>> Bounds are not a record of two pointers at least due to binary format.
>> You do not pass it as regular record, use special bound mode for whole
>> record. Why to use RECORD_TYPE for bounds if it'll never be handled as
>> regular RECORD_TYPE?
>
> Then you only can update it completely?  Seems awfully target specific
> for sth introduced at the GIMPLE level.  Oh well.
>
> Richard.
>
>> Ilya
>>
>>>
>>> Richard.
>>>
>>>> Ilya
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI usage results in missed optimizations.
>>>>>>  My original patch does not.
>>>>>>
>>>>>> Ilya
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Also sth weird is going on as your arg_bnd call gets [return slot optimization].
>>>>>>>>> It seems that your POINTER_BOUNDS_TYPE is seen as aggregate somehow.
>>>>>>>>
>>>>>>>> Target is responsible for having return slot optimization here. And
>>>>>>>> target expands all such calls. So, do not see the problem here.
>>>>>>>>
>>>>>>>> Ilya
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Richard.
>>>>>>>>>
>>>>>>>>>> Ilya
>>>>>>>>>>>
>>>>>>>>>>> Richard.
>>>>>>>>>>>


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