On Wed, Nov 6, 2013 at 3:04 PM, Ilya Enkovich <enkovich....@gmail.com> wrote:
> 2013/11/6 Richard Biener <richard.guent...@gmail.com>:
>> 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);
>
> 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.
>>
>>>>
>>>> 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.
>>>>>>>>>>
