[PATCH] D18822: [Polly][FIX] Adjust execution context of hoisted loads wrt. error domains

[Tobias Grosser via llvm-commits]

On 04/07/2016 03:44 PM, Johannes Doerfert wrote:
> On 04/07, Tobias Grosser wrote:
>> On 04/07/2016 03:36 PM, Johannes Doerfert wrote:
>>> On 04/07, Tobias Grosser wrote:
>>>> On 04/07/2016 01:32 PM, Johannes Doerfert wrote:
>>>>> On 04/07, Tobias Grosser wrote:
>>>>>> ================
>>>>>> Comment at: lib/Analysis/ScopInfo.cpp:3060
>>>>>> @@ -3046,1 +3059,3 @@
>>>>>>
>>>>>> +isl_set *Scop::getErrorCtxReachingStmt(ScopStmt &Stmt) {
>>>>>> +  auto *BB = Stmt.getEntryBlock();
>>>>>> ----------------
>>>>>> Add __isl_give here as well.
>>>>> We (or at least I) do not use isl memory annotations in the CPP file if
>>>>> it is not a static function. Apparently you want me to now but not
>>>>> before so I am confused.
>>>>
>>>> I may have missed this before. I would suggest to agree on something.
>>>>
>>>> $grep '^isl_' lib/*/*.cpp | wc
>>>>       14      69    1151
>>>>
>>>> $grep '^__isl_' lib/*/*.cpp | wc
>>>>       63     298    5010
>>>>
>>>> It seems we have both styles, with the second more common than the first. I
>>>> personally prefer the second style, as it makes this information readily
>>>> available when looking at the implementation of a function.
>>>>
>>>> Do you suggest to switch to your style instead?
>>
>>> I don't know. I just noticed I (usually) do not use the __isl_* in the
>>> cpp if it is not static, however that doesn't seem to be the case often.
>>> If you prefer the second style we can use that one, but then I will
>>> immediately raise the same question for arguments in the definition.
>>> Do we want:
>>>    ScopArrayInfo::getFromAccessFunction(__isl_keep isl_pw_multi_aff *PMA) {
>>>      ...
>>>    }
>>> or
>>>    ScopArrayInfo::getFromAccessFunction(isl_pw_multi_aff *PMA) {
>>>      ...
>>>    }
>>
>> I would go for the first. The same reasoning applies and it is also more
>> consistent.
> Ok.
>
>> If we agree on this,
> Good for me.
>
>> I propose to add the annotations at places where they are missing.
>> (Obviously, this is not coupled to this patch. Just a note for
>> myself).
> Let's do that at some point soon (in one commit) and from now on look
> for it in patches.

Sounds good.

Best,
Tobias