[PATCH] Update source location for PRE inserted stmt

[Dehao Chen]

On Sun, Nov 25, 2012 at 11:37 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Sun, Nov 25, 2012 at 4:40 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Nov 15, 2012 at 5:46 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>>>> But UNKNOWN_LOCATION is effectively wrong as well.  If other
>>>> optimizations move the statements above the inserted instruction, then
>>>> the new instruction ends up inheriting whatever location happens to be
>>>> in the previous statement.
>>>
>>> That's correct, UNKNOWN_LOCATION isn't a panacea either, although it can help
>>> in some cases.  The only safe thing to do is to put the exact source location
>>> of the statement, i.e. the location of the source construct for which the
>>> statement has been generated.
>>
>> There may not be a source location for a generated statement, the computed
>> value might not have been computed in the source at any point even.  Consider
>> re-association of an expression and then a re-associated part becoming
>> partially redundant.
>>
>>  if ()
>>    tem = a + b;
>>  tem2 = a + c  + b;
>>
>> after re-assoc + PRE:
>>
>>  if ()
>>    tem = a + b;
>> else
>>    tem' = a + b;  // which sloc here?

For inserted expr/stmt, inheriting location from placeholder is making
sense. But when the placeholder has no location in the first place,
explicit NOWHERE is needed, otherwise it will just has random location
from other basic block.

Thus for this cases, sloc here should be NOWHERE.

However, in the unittest I provided:

  if (x > 0)
    ret += *a;
  else
    a++;
  ret += *a;

After PRE:

if (x > 0) {
  pretmp_1 = *a;
  ret += pretmp_1;
} else {
  a++;
  pretmp_2 = *a; // it should inherit location from prev line
}
pretmp_3 = PHI(pretmp_1, pretmp_2)

How does that sound?

Thanks,
Dehao

>
> If there was a else branch, the proposed patch by Dehao will use the
> source location of the previous stmt 'tem' = a + b' was inserted
> after.
>
> I did not see any special source location handling in
> tree-ssa-reassoc.c -- looks like the same principle is used there --
> the new assignment after the operand shuffling will inherit the source
> location of the placeholder gimple stmt.
>
>
>> tem'' = PHI <tem, tem'>  //  or here?  on the args?
>> tem2 = tem'' + c;
>>
>> so what source location would you use for the inserted expression?  If
>> UNKNOWN is not
>> correct here then I think we need an explicit NOWHERE?



>
> Can that break line coverage info too, say, when source location get
> stripped from reassociated expressions?
>
> David
>
>
>
>
>>
>>>> Fixing the location when the statement is inserted will reduce this
>>>> randomness.  I'm not sure I understand why you think this will break
>>>> coverage.  The examples given in this thread were helped by this
>>>> location fixing.
>>>
>>> What do you call randomness exactly?  GDB jumpiness?  I'm a little wary of
>>> fixing GDB jumpiness by distorting the debug info.  Ian has clearly outlined
>>> the correct approach to addressing it.
>>>
>>> Note that I didn't specifically reply to Dehao's patch here, which might be
>>> acceptable in the end, only to David's seemingly general statement about the
>>> "natural way" of putting a location on these statements.
>>
>> Richard.
>>
>>> --
>>> Eric Botcazou