[llvm-commits] [LLVMdev] [polly] scev codegen (first step to remove the dependence on ivcanon pass)
Tobias Grosser
tobias at grosser.es
Thu Dec 13 13:25:56 PST 2012
On 12/10/2012 10:30 PM, Sebastian Pop wrote:
> Hi Tobi,
>
> here is the first part of removing the ivcanon pass: the two patches for llvm
> are implementing the scev apply function and the parameter rewriter (based on
> your previous code SCEVRewriter). On the Polly side the two patches are first
> collecting the LoopToScev map and using the apply function and the parameter
> rewriter.
Hi Sebastian,
this looks very nice. Some minor comments inline:
> 0001-add-SCEVParameterRewriter.patch
>
>
> From 328c0cd4c246768b73dff1f8f5a3c9a089b5fa64 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Fri, 7 Dec 2012 09:37:17 -0600
> Subject: [PATCH 1/2] add SCEVParameterRewriter
>
> ---
> include/llvm/Analysis/ScalarEvolutionExpressions.h | 104 ++++++++++++++++++++
> 1 file changed, 104 insertions(+)
>
> diff --git a/include/llvm/Analysis/ScalarEvolutionExpressions.h b/include/llvm/Analysis/ScalarEvolutionExpressions.h
> index b74cb33..48babff 100644
> --- a/include/llvm/Analysis/ScalarEvolutionExpressions.h
> +++ b/include/llvm/Analysis/ScalarEvolutionExpressions.h
> @@ -548,6 +548,110 @@ namespace llvm {
> SCEVTraversal<SV> T(Visitor);
> T.visitAll(Root);
> }
> +
> + /// The ScevRewriter takes a scalar evolution expression and copies all its
> + /// components. The result after a rewrite is an identical SCEV.
> + struct ScevRewriter
> + : public SCEVVisitor<ScevRewriter, const SCEV*> {
> + public:
> + ScevRewriter(ScalarEvolution &S) : SE(S) {}
> +
> + virtual const SCEV *visitConstant(const SCEVConstant *Constant) {
> + return Constant;
> + }
> +
> + virtual const SCEV *visitTruncateExpr(const SCEVTruncateExpr *Expr) {
> + const SCEV *Operand = visit(Expr->getOperand());
> + return SE.getTruncateExpr(Operand, Expr->getType());
> + }
> +
> + virtual const SCEV *visitZeroExtendExpr(const SCEVZeroExtendExpr *Expr) {
> + const SCEV *Operand = visit(Expr->getOperand());
> + return SE.getZeroExtendExpr(Operand, Expr->getType());
> + }
> +
> + virtual const SCEV *visitSignExtendExpr(const SCEVSignExtendExpr *Expr) {
> + const SCEV *Operand = visit(Expr->getOperand());
> + return SE.getSignExtendExpr(Operand, Expr->getType());
> + }
> +
> + virtual const SCEV *visitAddExpr(const SCEVAddExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> + return SE.getAddExpr(Operands);
> + }
> +
> + virtual const SCEV *visitMulExpr(const SCEVMulExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> + return SE.getMulExpr(Operands);
> + }
> +
> + virtual const SCEV *visitUDivExpr(const SCEVUDivExpr *Expr) {
> + return SE.getUDivExpr(visit(Expr->getLHS()), visit(Expr->getRHS()));
> + }
> +
> + virtual const SCEV *visitAddRecExpr(const SCEVAddRecExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> + return SE.getAddRecExpr(Operands, Expr->getLoop(),
> + Expr->getNoWrapFlags());
> + }
> +
> + virtual const SCEV *visitSMaxExpr(const SCEVSMaxExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> + return SE.getSMaxExpr(Operands);
> + }
> +
> + virtual const SCEV *visitUMaxExpr(const SCEVUMaxExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> + return SE.getUMaxExpr(Operands);
> + }
> +
> + virtual const SCEV *visitUnknown(const SCEVUnknown *Expr) {
> + return Expr;
> + }
> +
> + virtual const SCEV *visitCouldNotCompute(const SCEVCouldNotCompute *Expr) {
> + return Expr;
> + }
> +
> + protected:
> + ScalarEvolution &SE;
> + };
> +
> + typedef DenseMap<const Value*, Value*> ValueToValueMap;
> +
> + /// 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.
Also, we see if there are any concerns from the SCEV authors.
> #endif
> -- 1.7.9.5
>
>
> 0002-add-ScevApplyRewriter.patch
>
>
> From 455451da4f213a9a6f9f12565331a7a1d86ee5e5 Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Sat, 8 Dec 2012 10:16:49 -0600
> Subject: [PATCH 2/2] add ScevApplyRewriter
>
> ---
> include/llvm/Analysis/ScalarEvolutionExpressions.h | 39 ++++++++++++++++++++
> 1 file changed, 39 insertions(+)
>
> diff --git a/include/llvm/Analysis/ScalarEvolutionExpressions.h b/include/llvm/Analysis/ScalarEvolutionExpressions.h
> index 48babff..f9ed59e 100644
> --- a/include/llvm/Analysis/ScalarEvolutionExpressions.h
> +++ b/include/llvm/Analysis/ScalarEvolutionExpressions.h
> @@ -652,6 +652,45 @@ namespace llvm {
> ValueToValueMap ⤅
> };
>
> + typedef DenseMap<const Loop*, const SCEV*> LoopToScevMapT;
> +
> + /// 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.
> + struct ScevApplyRewriter: public ScevRewriter {
> + public:
> + static const SCEV *rewrite(const SCEV *Scev, LoopToScevMapT &Map,
> + ScalarEvolution &SE) {
> + ScevApplyRewriter Rewriter(SE, Map);
> + return Rewriter.visit(Scev);
> + }
> + ScevApplyRewriter(ScalarEvolution &S, LoopToScevMapT &M)
> + : ScevRewriter(S), Map(M) {}
> +
> + virtual const SCEV *visitAddRecExpr(const SCEVAddRecExpr *Expr) {
> + SmallVector<const SCEV *, 2> Operands;
> + for (int i = 0, e = Expr->getNumOperands(); i < e; ++i)
> + Operands.push_back(visit(Expr->getOperand(i)));
> +
> + const Loop *L = Expr->getLoop();
> + const SCEV *Res = SE.getAddRecExpr(Operands, L, Expr->getNoWrapFlags());
> +
> + if (0 == Map.count(L))
> + return Res;
> +
> + const SCEVAddRecExpr *Rec = (const SCEVAddRecExpr *) Res;
> + return Rec->evaluateAtIteration(Map[L], SE);
> + }
> +
> + private:
> + LoopToScevMapT ⤅
> + };
> +
> +/// 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.
Again, we should see if there any concerns from the scev experts.
> 0001-add-LoopToScev-maps.patch
>
>
> From de79e54850b9c8433a0b28e32f51fd305737e87f Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Mon, 3 Dec 2012 11:54:43 -0600
> Subject: [PATCH 1/2] add LoopToScev maps
Yes, this one is straightforward.
> 0002-use-apply-and-ScevParameterRewriter-rewrite-instead-.patch
>
>
> From 968c590024c4b1526e982ba2a6099d7b0c9ea3dc Mon Sep 17 00:00:00 2001
> From: Sebastian Pop<spop at codeaurora.org>
> Date: Mon, 10 Dec 2012 13:33:51 -0600
> Subject: [PATCH 2/2] use apply and ScevParameterRewriter::rewrite instead of
> SCEVRewriter
This is also a very nice simplification.
> 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?
Cheers
Tobi
More information about the llvm-commits
mailing list