[Polly][PATCH 4/8] Allow the IslExprBuilder to compare pointers

[Tobias Grosser]

On 11/08/2014 10:56, Johannes Doerfert wrote:
> On 08/11, Tobias Grosser wrote:
>> On 11/08/2014 09:38, Johannes Doerfert wrote:
>>> On 08/11, Tobias Grosser wrote:
>>>> Please merge this into the patch that actually tests and uses it.
>>>>
>>>> Some comments inline:
>>>>
>>>> On 10/08/2014 09:50, Johannes Doerfert wrote:
>>>> [..]
>>>>>     switch (isl_ast_expr_get_op_type(Expr)) {
>>>>>     default:
>>>>> @@ -280,16 +286,20 @@ Value *IslExprBuilder::createOpICmp(__isl_take isl_ast_expr *Expr) {
>>>>>       Res = Builder.CreateICmpEQ(LHS, RHS);
>>>>>       break;
>>>>>     case isl_ast_op_le:
>>>>> -    Res = Builder.CreateICmpSLE(LHS, RHS);
>>>>> +    Res = IsPtrType ? Builder.CreateICmpULE(LHS, RHS)
>>>>> +                    : Builder.CreateICmpSLE(LHS, RHS);
>>>>>       break;
>>>>>     case isl_ast_op_lt:
>>>>> -    Res = Builder.CreateICmpSLT(LHS, RHS);
>>>>> +    Res = IsPtrType ? Builder.CreateICmpULT(LHS, RHS)
>>>>> +                    : Builder.CreateICmpSLT(LHS, RHS);
>>>>>       break;
>>>>>     case isl_ast_op_ge:
>>>>> -    Res = Builder.CreateICmpSGE(LHS, RHS);
>>>>> +    Res = IsPtrType ? Builder.CreateICmpUGE(LHS, RHS)
>>>>> +                    : Builder.CreateICmpSGE(LHS, RHS);
>>>>>       break;
>>>>>     case isl_ast_op_gt:
>>>>> -    Res = Builder.CreateICmpSGT(LHS, RHS);
>>>>> +    Res = IsPtrType ? Builder.CreateICmpUGT(LHS, RHS)
>>>>> +                    : Builder.CreateICmpSGT(LHS, RHS);
>>>>
>>>> Did you consider CreateICmp(Predicate, LHS, RHS), plus a table that
>>>> maps the enum values to the corresponding predicates? This code seems
>>>> to start to have a lot of duplication.
>>> I did not really, but we could use a two dimensional table if you prefer that.
>>
>> Does a 2D table work? I think we would need a map for this, as the index is
>> an enum, no?
> Yes, isl enums = C enums = int
>
>> Otherwise, we could just make this two switch statements, that select
>> the ICmp predicate and than just have a single call to Builder.CreateImp at
>> the end. What do you think?
> I don't have any strong feeling about it, or in other words I think
> that's a lot of discussion about 10 lines of code which just builds a
> compare instruction.
>
> Tell me what you want and I change it to whatever.

I would pull out the Builder.Create* from the switch statement and 
instead just select the predicate.

We can then use Builder.CreateICmp(Predicate, LHS, RHS) after the switch.

Cheers,
Tobias