[llvm-commits] [LLVMdev] [polly] scev codegen (first step to remove the dependence on ivcanon pass)

[Tobias Grosser]

On 12/13/2012 11:09 PM, Sebastian Pop wrote:
> Hi,
>
> Tobias Grosser wrote:
>>> +  /// The ScevParameterRewriter takes a scalar evolution expression and updates
>>> +  /// the SCEVUnknown components following the Map (Value -> Value).
>>> +  struct ScevParameterRewriter: public ScevRewriter {
>>> +  public:
>>> +    static const SCEV *rewrite(const SCEV *Scev, ScalarEvolution &SE,
>>> +                               ValueToValueMap &Map) {
>>> +      ScevParameterRewriter Rewriter(SE, Map);
>>> +      return Rewriter.visit(Scev);
>>> +    }
>>> +    ScevParameterRewriter(ScalarEvolution &S, ValueToValueMap &M)
>>> +      : ScevRewriter(S), Map(M) {}
>>> +
>>> +    virtual const SCEV *visitUnknown(const SCEVUnknown *Expr) {
>>> +      Value *V = Expr->getValue();
>>> +      if (Map.count(V))
>>> +        return SE.getUnknown(Map[V]);
>>> +      return Expr;
>>> +    }
>>> +
>>> +  private:
>>> +    ValueToValueMap ⤅
>>> +  };
>>> +
>>
>> This one looks good. However, you may want to add some unit tests.
>
> As this functionality is only used in Polly right now, I don't know how to write
> test cases in LLVM.

I was thinking of unittests/Analysis/ScalarEvolutionTest.cpp

However, it seems such tests are not very reliable:
http://llvm.org/bugs/show_bug.cgi?id=11052

>>> +  /// The ScevApplyRewriter takes a scalar evolution expression and applies
>>> +  /// the Map (Loop -> SCEV) to all AddRecExprs.
>>
>> The word 'apply' in the class name sounds very generic. Is it in
>> some way specific to Loops, or could we just call it
>> ScevLoopRewriter.
>
> [...]
>
>>> +
>>> +/// Applies the Map (Loop -> SCEV) to the given Scev.
>>> +static inline const SCEV *apply(const SCEV *Scev, LoopToScevMapT &Map,
>>> +                                ScalarEvolution &SE) {
>>> +  return ScevApplyRewriter::rewrite(Scev, Map, SE);
>>> +
>>
>> I don't think that the 'apply' function gives any benefit. Calling
>> ScevLoopRewriter::rewrite() seems to be a lot more clear.
>
> apply is the correct word for what this function does: it takes a function (the
> scev) and applies the map to each dimension it variates in.

Thanks for pointing out that apply is the right term. I am fine with 
leaving it. However, you might still move this function in the 
ScevApplyRewriter.

>>> diff --git a/test/Cloog/CodeGen/simple_vec_call_2.ll b/test/Cloog/CodeGen/simple_vec_call_2.ll
>>> index bb2a680..645599f 100644
>>> --- a/test/Cloog/CodeGen/simple_vec_call_2.ll
>>> +++ b/test/Cloog/CodeGen/simple_vec_call_2.ll
>>> @@ -58,4 +58,4 @@ return:
>>>   ; CHECK-SCEV: %5 = insertelement <4 x float**> %4, float** %p_result1, i32 1
>>>   ; CHECK-SCEV: %6 = insertelement <4 x float**> %5, float** %p_result2, i32 2
>>>   ; CHECK-SCEV: %7 = insertelement <4 x float**> %6, float** %p_result3, i32 3
>>> -; CHECK-SCEV: store <4 x float**> %7, <4 x float**>* bitcast ([1024 x float**]* @B to <4 x float**>*), align
>>> +; CHECK-SCEV: store <4 x float**> %7, <4 x float**>* bitcast (float*** getelementptr inbounds ([1024 x float**]* @B, i64 0, i64 3) to <4 x float**>*), align
>>
>> I don't understand this test case change. As I am especially
>> surprised due to the second operand of the getelementptr. Why is it
>> 'i64 3' and not 'i64 0'? Does this change change the semantics of
>> the generated code?
>
> I was puzzled by this change as well: the 4 patches are not supposed to change
> the semantics of the vectorizer: it looks like what changes is the store access
> function, from a reference to B to a gep in B.  I will investigate a bit more.

Changing from the reference to a gep is not worrying me. The surprising 
thing from my point is that there are non-zero coefficients in the gep.

Cheers
Tobi