[Polly][Refactor] IslAst [2/7]
Johannes Doerfert
jdoerfert at codeaurora.org
Wed Jul 23 11:35:00 PDT 2014
r213779
Thanks again for you reviews guys! Very good comments (I addressed them all)!
--
Johannes Doerfert
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
From: Yabin Hu [mailto:yabin.hwu at gmail.com]
Sent: Wednesday, July 23, 2014 10:10 AM
To: Johannes Doerfert
Cc: Tobias Grosser; simbuerg at fim.uni-passau.de; llvm-commits at cs.uiuc.edu; Sebastian Pop
Subject: Re: [Polly][Refactor] IslAst [2/7]
Hi Johannes,
Could somebody review this patch?
Best regards,
Johannes
I told Tobias that I would try to review the patch several days before. I am very sorry for the delay.
Overall, the patch looks very good to me. Some comments inline.
>From 783bdc0496e6614d135a5eb875fb8072954861a1 Mon Sep 17 00:00:00 2001
From: Johannes Doerfert <jdoerfert at codeaurora.org>
Date: Wed, 16 Jul 2014 11:56:31 -0700
Subject: [PATCH 2/7] [Refactor] Unify IslAst print methods
+ Add const annotations to some member functions
---
include/polly/CodeGen/IslAst.h | 21 +++---
lib/CodeGen/IslAst.cpp | 90 +++++++++++--------------
test/Isl/CodeGen/single_loop_zero_iterations.ll | 2 +-
3 files changed, 52 insertions(+), 61 deletions(-)
diff --git a/include/polly/CodeGen/IslAst.h b/include/polly/CodeGen/IslAst.h
index 28bb051..79ac285 100644
--- a/include/polly/CodeGen/IslAst.h
+++ b/include/polly/CodeGen/IslAst.h
@@ -54,17 +54,21 @@ struct IslAstUserPayload {
};
class IslAstInfo : public ScopPass {
- Scop *S;
- IslAst *Ast;
+ Scop *S = nullptr;
+ IslAst *Ast = nullptr;
public:
static char ID;
- IslAstInfo() : ScopPass(ID), Ast(NULL) {}
+ IslAstInfo() : ScopPass(ID) {}
- /// Print a source code representation of the program.
- void pprint(llvm::raw_ostream &OS);
+ /// @brief Build the AST for the given SCoP @p S
Missing a dot at the end of the line.
+ bool runOnScop(Scop &S);
+
+ /// @brief Print a source code representation of the program.
+ void printScop(llvm::raw_ostream &OS) const;
- isl_ast_node *getAst();
+ /// @brief Return a copy of the AST root node
Missing a dot at the end of the line.
+ __isl_give isl_ast_node *getAst() const;
/// @brief Get the run conditon.
///
@@ -72,10 +76,7 @@ public:
/// assumptions that have been taken hold. If the run condition evaluates to
/// zero/false some assumptions do not hold and the original code needs to
/// be executed.
- __isl_give isl_ast_expr *getRunCondition();
-
- bool runOnScop(Scop &S);
- void printScop(llvm::raw_ostream &OS) const;
+ __isl_give isl_ast_expr *getRunCondition() const;
/// @name Extract information attached to an isl ast (for) node
///
diff --git a/lib/CodeGen/IslAst.cpp b/lib/CodeGen/IslAst.cpp
index 6479e8d..4302758 100644
--- a/lib/CodeGen/IslAst.cpp
+++ b/lib/CodeGen/IslAst.cpp
@@ -392,14 +392,6 @@ IslAst::IslAst(Scop *Scop, Dependences &D) : S(Scop) {
isl_union_map *Schedule =
isl_union_map_intersect_domain(S->getSchedule(), S->getDomains());
- Function *F = Scop->getRegion().getEntry()->getParent();
- (void)F;
-
- DEBUG(dbgs() << ":: isl ast :: " << F->getName()
- << " :: " << Scop->getRegion().getNameStr() << "\n");
-
- DEBUG(dbgs() << S->getContextStr() << "\n"; isl_union_map_dump(Schedule));
-
if (DetectParallel || PollyVectorizerChoice != VECTORIZER_NONE) {
BuildInfo.Deps = &D;
BuildInfo.InParallelFor = 0;
@@ -415,8 +407,6 @@ IslAst::IslAst(Scop *Scop, Dependences &D) : S(Scop) {
Root = isl_ast_build_ast_from_schedule(Context, Schedule);
isl_ast_build_free(Context);
-
- DEBUG(pprint(dbgs()));
}
IslAst::~IslAst() {
@@ -424,41 +414,11 @@ IslAst::~IslAst() {
isl_ast_expr_free(RunCondition);
}
-/// Print a C like representation of the program.
-void IslAst::pprint(llvm::raw_ostream &OS) {
- isl_ast_node *Root;
- isl_ast_print_options *Options;
-
- Options = isl_ast_print_options_alloc(S->getIslCtx());
- Options = isl_ast_print_options_set_print_for(Options, &printFor, nullptr);
-
- isl_printer *P = isl_printer_to_str(S->getIslCtx());
- P = isl_printer_set_output_format(P, ISL_FORMAT_C);
-
- P = isl_printer_print_ast_expr(P, RunCondition);
- char *result = isl_printer_get_str(P);
- P = isl_printer_flush(P);
-
- OS << "\nif (" << result << ")\n\n";
- P = isl_printer_indent(P, 4);
-
- Root = getAst();
- P = isl_ast_node_print(Root, P, Options);
- result = isl_printer_get_str(P);
- OS << result << "\n";
- OS << "else\n";
- OS << " { /* original code */ }\n\n";
- isl_printer_free(P);
- isl_ast_node_free(Root);
-}
-
__isl_give isl_ast_node *IslAst::getAst() { return isl_ast_node_copy(Root); }
__isl_give isl_ast_expr *IslAst::getRunCondition() {
return isl_ast_expr_copy(RunCondition);
}
-void IslAstInfo::pprint(llvm::raw_ostream &OS) { Ast->pprint(OS); }
-
void IslAstInfo::releaseMemory() {
if (Ast) {
delete Ast;
@@ -476,22 +436,15 @@ bool IslAstInfo::runOnScop(Scop &Scop) {
Ast = new IslAst(&Scop, D);
+ DEBUG(printScop(dbgs()));
return false;
}
-__isl_give isl_ast_node *IslAstInfo::getAst() { return Ast->getAst(); }
-__isl_give isl_ast_expr *IslAstInfo::getRunCondition() {
+__isl_give isl_ast_node *IslAstInfo::getAst() const { return Ast->getAst(); }
+__isl_give isl_ast_expr *IslAstInfo::getRunCondition() const {
return Ast->getRunCondition();
}
-void IslAstInfo::printScop(raw_ostream &OS) const {
- Function *F = S->getRegion().getEntry()->getParent();
-
- OS << F->getName() << "():\n";
-
- Ast->pprint(OS);
-}
-
IslAstUserPayload *IslAstInfo::getNodePayload(__isl_keep isl_ast_node *Node) {
isl_id *Id = isl_ast_node_get_annotation(Node);
if (!Id)
@@ -523,6 +476,43 @@ bool IslAstInfo::isReductionParallel(__isl_keep isl_ast_node *Node) {
return Payload && Payload->IsReductionParallel;
}
+void IslAstInfo::printScop(raw_ostream &OS) const {
+ isl_ast_print_options *Options;
+ isl_ast_node *RootNode = getAst();
+ isl_ast_expr *RunCondition = getRunCondition();
+ char *RtCStr, *AstStr;
+
+ Scop &S = getCurScop();
+ Options = isl_ast_print_options_alloc(S.getIslCtx());
+ Options = isl_ast_print_options_set_print_for(Options, printFor, nullptr);
+
+ isl_printer *P = isl_printer_to_str(S.getIslCtx());
+ P = isl_printer_print_ast_expr(P, RunCondition);
+ RtCStr = isl_printer_get_str(P);
+ P = isl_printer_flush(P);
+ P = isl_printer_indent(P, 4);
+ P = isl_printer_set_output_format(P, ISL_FORMAT_C);
+ P = isl_ast_node_print(RootNode, P, Options);
+ AstStr = isl_printer_get_str(P);
+
+ Function *F = S.getRegion().getEntry()->getParent();
+ isl_union_map *Schedule =
+ isl_union_map_intersect_domain(S.getSchedule(), S.getDomains());
+
+ OS << ":: isl ast :: " << F->getName() << " :: " << S.getRegion().getNameStr()
+ << "\n";
+ OS << S.getContextStr() << "\n";
+ isl_union_map_dump(Schedule);
Should we avoid use isl_*_dump in this function? Everything prints to OS, but _dump prints to stderr, right?
+ OS << "\nif (" << RtCStr << ")\n\n";
+ OS << AstStr << "\n";
+ OS << "else\n";
+ OS << " { /* original code */ }\n\n";
+
+ isl_ast_expr_free(RunCondition);
+ isl_union_map_free(Schedule);
+ isl_ast_node_free(RootNode);
+ isl_printer_free(P);
+}
Missing a separate blank line.
void IslAstInfo::getAnalysisUsage(AnalysisUsage &AU) const {
// Get the Common analysis usage of ScopPasses.
ScopPass::getAnalysisUsage(AU);
diff --git a/test/Isl/CodeGen/single_loop_zero_iterations.ll b/test/Isl/CodeGen/single_loop_zero_iterations.ll
index 48403f5..50036e6 100644
--- a/test/Isl/CodeGen/single_loop_zero_iterations.ll
+++ b/test/Isl/CodeGen/single_loop_zero_iterations.ll
@@ -66,5 +66,5 @@ return: ; preds = %if.else, %if.then
}
; CHECK: for region: 'for.cond => for.end.region' in function 'main':
-; CHECK-NEXT: main():
+; CHECK-NEXT: main
I personally would suggest use more complete output info: ":: isl ast :: main" instead of just "main" here.
; CHECK-NOT: Stmt_for_body(0);
--
1.8.2.1
Otherwise, looks good to me.
Regards,
Yabin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140723/f0eff73a/attachment.html>
More information about the llvm-commits
mailing list