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 [16/25] Tail recursion


On Wed, Nov 20, 2013 at 11:41 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> 2013/11/20 Richard Biener <richard.guenther@gmail.com>:
>> On Tue, Nov 19, 2013 at 8:02 PM, Jeff Law <law@redhat.com> wrote:
>>> On 11/18/13 14:03, Ilya Enkovich wrote:
>>>>
>>>> 2013/11/19 Jeff Law <law@redhat.com>:
>>>>>
>>>>> On 11/18/13 12:16, Ilya Enkovich wrote:
>>>>>>
>>>>>>
>>>>>> With current recursion elimination we will have:
>>>>>>
>>>>>> test (int *param1)
>>>>>> {
>>>>>> <bb1>:
>>>>>>
>>>>>> <bb2>:
>>>>>>     _7 = PHI<param1(D) (bb1), _6 (bb2)>
>>>>>>     bounds2 = __builtin_arg_bounds (_7) -- WRONG
>>>>>
>>>>>
>>>>> I wouldn't say it's wrong.  It's conservatively correct if properly
>>>>> implemented.   Why precisely do you consider this wrong?  If your code
>>>>> can't
>>>>> handle it, it really should be fixed.
>>>>
>>>>
>>>> It is wrong because __builtin_arg_bounds is used to get bounds of
>>>> input parameter and PHI node here is not an input parameter.
>>>
>>> OK, now we're getting somewhere.  So we've got this odd little function
>>> which only works on parameters.   I can't help but feel this is a bit of
>>> mis-design coming back to haunt us and I wonder if we're going to see other
>>> instances of this kind of problem.
>>
>> Right.
>>
>>> There's something just wrong with the semantics of __builtin_arg_bounds, but
>>> I'm having trouble putting my finger on it.
>>
>> Well, the issue is that it accepts any pointer argument as input.
>> I originally thought it may just get an integer constant argument - the
>> argument position.  It really seems to me that we have
>> __builtin_arg_boundsN (), but to avoid having N builtins we specify N
>> via an argument to the function.  But as it may not change we should
>> use something that cannot change - like a constant.  A constant
>> that identifies a parameter is one that gives its position for example.
>
> I have tried to pass constant for arg_bounds.  It still requires
> additional modification of optimization passes (including tail
> recursion, param propagation, inline etc.)  But having constant as an
> argument you really cannot detect errors.  E.g. if you inline and do
> not fix inlined arg_bounds calls correctly, you may not get ICE but
> get wrong program which uses wrong bounds and produces false bounds
> violations.

Yeah, that's why I refrained from suggesting this earlier ;)  You lose
the connection between "bounds for argument X" and "function entry
value for argument X".

Richard.

>>
>> Note that any restrictions you impose on what is "valid" for GIMPLE
>> should be verified in tree-cfg.c:verify_gimple_* - in the
>> __builtin_arg_bounds call case in verify_gimple_call.
>
> Will add it.
>
> Thanks,
> Ilya
>
>>
>> Richard.
>>
>>>
>>>  Correctl
>>>>
>>>> handling of __builtin_arg_bounds in this 'wrong' example requires
>>>> adding it a PHI node semantics based on it's arg.  For me it seems
>>>> more complex then generating a regular PHI node for bounds and makes
>>>> GIMPLE less readable.
>>>
>>> But presumably you have code to merge bound information already, right?  You
>>> need that for PHI nodes.  Can't you use that to walk up the use-def chains
>>> and build the bounds information?
>>>
>>> Or if you want to fix the tailcall stuff, you can start down that direction.
>>> I don't think that'll fix the nagging concerns about the overall semantics
>>> of builtin_arg_bounds, but it might be enough for you to go forward.
>>>
>>> jeff


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