[PATCH] [Polly][Refactor] Generalize parallel code generation

[Tobias Grosser]

On 03/10/2014 21:02, Johannes Doerfert wrote:
> On 10/03, Tobias Grosser wrote:
>> On 03/10/2014 20:28, Johannes Doerfert wrote:
>>>>> ! In D4990#17, @grosser wrote:
>>>> I like the idea with the DataLayout. This makes the behaviour a lot more clear. Thanks for insisting here.
>>>>
>>>> You also sneaked in a semantic change without a test case (actually it can not be tested as the current CLooG code generator always generates matching types making this basically dead code). I would feel more comfortable if we remove such asserts only when needed and with a specific test case and explaining why truncating is OK, instead of having this part of a larger refactoring.
>>>>
>>>> 	​ if (LB->getType() != LongType)
>>>> 	​ LB = Builder.CreateSExtOrTrunc(LB, LongType);
>>>> 	​ if (UB->getType() != LongType)
>>>> 	​ UB = Builder.CreateSExtOrTrunc(UB, LongType);
>>>> 	​ if (Stride->getType() != LongType)
>>>> 	​ Stride = Builder.CreateSExtOrTrunc(Stride, LongType);
>>>
>>> What exactly is your problem here?
>>
>> 1) This code is untested (and to my understanding dead)
>>
>> 2) Before your change we would throw an assertion in this case,
>>     now we silently truncate values.
>>
>>     (Even if your asserts have not been around, the IRBuilder would have
>>      thrown an assertion)
>>
>>> The type asserts were introduced by my patch and are replaced by this
>>> because we need this anyway.
>>>
>>> What else do we want to do if type(LB) <
>>> LongType or type(LB) > LongType?
>>
>> This case does not happen at the moment. At the moment the isl or cloog code
>> generation is modified to generate such code (for whatever reason),
>> I would prefer to be notified, such that we can think about if truncating
>> the values is save and that we can commit a corresponding test case.
>>
>> You said we need this anyway. I am honestly a little surprised this is
>> needed at the moment. Maybe I missed something. What is it needed for?
> I can at least imagine a machine/OS with LongType != i64.
>
> AFAIK: E.g., The C standard doesn't have a problem with long == int == i32.

The CLooG code generation derives the types it uses with the same 
mechanism than the OpenMP code generation. So on these architectures all 
types should be i32.

You are very right, there are situations where we may have different 
types (e.g. the isl code generation uses always 64 bit, hence we may 
come to this soon). I am not saying that we never need to address this. 
I just propose to reason about this at the moment where we run in this 
situation and the code is actually needed and testable.

> However, I'll remove the code for now and we go back to the original
> behaviour.

Nice. I am looking forward to your patch to rebase the OpenMP patch I 
have on my side.

Cheers
Tobias