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

[Tobias Grosser via llvm-commits]

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.

If we agree on this, 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).

Best,
Tobias