On Wed, Nov 6, 2013 at 2:31 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
> 2013/11/6 Richard Biener <richard.guent...@gmail.com>:
>> On Wed, Nov 6, 2013 at 12:59 PM, Ilya Enkovich <enkovich....@gmail.com>
>> wrote:
>>> 2013/11/6 Richard Biener <richard.guent...@gmail.com>:
>>>> On Wed, Nov 6, 2013 at 12:00 PM, Ilya Enkovich <enkovich....@gmail.com>
>>>> wrote:
>>>>> 2013/11/6 Richard Biener <richard.guent...@gmail.com>:
>>>>>> On Tue, Nov 5, 2013 at 3:24 PM, Ilya Enkovich <enkovich....@gmail.com>
>>>>>> wrote:
>>>>>>> 2013/11/5 Richard Biener <richard.guent...@gmail.com>:
>>>>>>>> On Tue, Nov 5, 2013 at 1:52 PM, Ilya Enkovich <enkovich....@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);
>>
>> 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.
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.
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.
>>>>>>>>
