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

[Sebastian Pop]

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.

> >+  /// 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.

> >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.

Thanks for your review.
Sebastian
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation