Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D5076 Diff 12967 lib/CodeGen/IslCodeGeneration.cpp

Changeset View

Standalone View

View Options

lib/CodeGen/IslCodeGeneration.cpp

Show First 20 Lines • Show All 562 Lines • ▼ Show 20 Lines
namespace { namespace {
class IslCodeGeneration : public ScopPass { class IslCodeGeneration : public ScopPass {
public: public:
static char ID; static char ID;
IslCodeGeneration() : ScopPass(ID) {} IslCodeGeneration() : ScopPass(ID) {}
/// @name The analysis passes we need to generate code.
///
///{
LoopInfo *LI;
IslAstInfo *AI;
DominatorTree *DT;
ScalarEvolution *SE;
///}
grosserUnsubmitted
Not Done
ReplyInline Actions

Making these analysis pass definitions class variables seems a conceptually independent change. It possibly does not hurt in this patch, but if you believe tracking those analysis passes as class variables is better style, I wonder if we should not have an independent patch that does this kind of transformation all over Polly. This patch would help to educate other people about the reasons this style is preferred and could also be a reference in future patch reviews.

There are other similar uses in ScheduleOptimizer.cpp, ScopInfo.cpp.

Also, before writing such a patch, it may be good to discuss the reason why this change is preferred. I personally try to always make the life time of a variable as short as possible. This change goes against this goal. So it would be good to understand why you believe this is better? There may be very good reasons, which I may have missed. Is this e.g. to align our style with LLVM? Or just to have a more consistent style in Polly (our code is rather inconsistent)?

grosser: Making these analysis pass definitions class variables seems a conceptually independent change.
jdoerfertAuthorUnsubmitted
Not Done
ReplyInline Actions

A few thoughts to this commit (not all related to each other):

  • Why do I need to extract non invasiv move of 4 variables in a refactoring patch? At some point the amount of work I'm supposed to put into "cleaning" my patches becomes ridiculous, especially compared to what else goes in.
  • Polly is inconsistent here, we have both styles, local variables and class members, however the latter can "always" be used.
  • The class I edit here has one function,...one. Where these variables are defined (in this one function) or as a memeber will only change the kind of comment we can add to their declaration.
jdoerfert: A few thoughts to this commit (not all related to each other): - Why do I need to extract…
/// @brief The loop anotator to generate llvm.loop metadata.
grosserUnsubmitted
Not Done
ReplyInline Actions

typo: annotator

grosser: typo: annotator
LoopAnnotator Annotator;
grosserUnsubmitted
Not Done
ReplyInline Actions

By moving the LoopAnnotator into class scop, we extend its lifetime. This means the code generation of different scops will use the same loop annotator. Hence, in case an earlier scop leaves the loop annotator in non-clear state, this may impact later scops.

Was this intentional?

grosser: By moving the LoopAnnotator into class scop, we extend its lifetime. This means the code…
jdoerfertAuthorUnsubmitted
Not Done
ReplyInline Actions

Yes.

I'd say:

Don't leave anything in an unclean state and comment your variables like:
  "LoobAnnotator Annotator;"

Is there any reason (except the state thing) that would benefit from constructing a new one all the time?

jdoerfert: Yes. I'd say: Don't leave anything in an unclean state and comment your variables like…
grosserUnsubmitted
Not Done
ReplyInline Actions

I don't think the overhead of (re)constructing it is measurable.

In terms of state, I was not talking about clean, but clear. Assuming, the LoopAnnotator
would contain a std::set, which would be _cleared_ on destruction, your change could cause old pointers to remain in this set unintended.

Keeping the live-time of variables short, avoids the need to think about such changes. In this case I looked into the LoopAnnotator and moving it seems to not break any assumptions.

grosser: I don't think the overhead of (re)constructing it is measurable. In terms of state, I was not…
/// @brief Build the delinearization runtime condition.
///
/// Build the condition that evaluates at run-time to true iff all
/// delinearization assumptions taken for the SCoP hold, and to zero
/// otherwise.
///
dpeixottUnsubmitted
Not Done
ReplyInline Actions

typo save/unsave -> safe/unsafe

dpeixott: typo save/unsave -> safe/unsafe
/// @return A value evaluating to true/false if delinarization is save/unsave.
Value *buildRTC(PollyIRBuilder &Builder, IslExprBuilder &ExprBuilder) {
Builder.SetInsertPoint(Builder.GetInsertBlock()->getTerminator());
Value *RTC = ExprBuilder.create(AI->getRunCondition());
return Builder.CreateIsNotNull(RTC);
grosserUnsubmitted
Not Done
ReplyInline Actions

Creating a new function for run time condition handling makes this code a lot more readable.

Two minor remarks:

  • Please use RunTimeCondition instead of RTC, as this is not really a common expression.
  • Run time conditions can conceptually be used (and hopefully will be used) for a lot more than delinearization. Hence, calling it delinearization condition is misleading. However, we could still use delinearization as an example or what a run time condition is or where it is used.
grosser: Creating a new function for run time condition handling makes this code a lot more readable.
jdoerfertAuthorUnsubmitted
Not Done
ReplyInline Actions
  • You don't like my variables names (even though they are clear abrivations of the full names) and now I have to change that?
  • At the moment there is only delinearization, why should I paint a picture of the future in the description of a feature?
jdoerfert: - You don't like my variables names (even though they are clear abrivations of the full…
}
bool runOnScop(Scop &S) { bool runOnScop(Scop &S) {
LoopInfo &LI = getAnalysis<LoopInfo>(); LI = &getAnalysis<LoopInfo>();
IslAstInfo &AstInfo = getAnalysis<IslAstInfo>(); AI = &getAnalysis<IslAstInfo>();
ScalarEvolution &SE = getAnalysis<ScalarEvolution>(); DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree(); SE = &getAnalysis<ScalarEvolution>();
assert(!S.getRegion().isTopLevelRegion() && assert(!S.getRegion().isTopLevelRegion() &&
"Top level regions are not supported"); "Top level regions are not supported");
simplifyRegion(&S, this); BasicBlock *EnteringBB = simplifyRegion(&S, this);
PollyIRBuilder Builder = createPollyIRBuilder(EnteringBB, Annotator);
BasicBlock *StartBlock = executeScopConditionally(S, this);
isl_ast_node *Ast = AstInfo.getAst();
LoopAnnotator Annotator;
PollyIRBuilder Builder(StartBlock->getContext(), llvm::ConstantFolder(),
polly::IRInserter(Annotator));
Builder.SetInsertPoint(StartBlock->begin());
IslNodeBuilder NodeBuilder(Builder, Annotator, this, LI, SE, DT); IslNodeBuilder NodeBuilder(Builder, Annotator, this, *LI, *SE, *DT);
Builder.SetInsertPoint(StartBlock->getSinglePredecessor()->begin());
NodeBuilder.addMemoryAccesses(S); NodeBuilder.addMemoryAccesses(S);
NodeBuilder.addParameters(S.getContext()); NodeBuilder.addParameters(S.getContext());
// Build condition that evaluates at run-time if all assumptions taken
// for the scop hold. If we detect some assumptions do not hold, the Value *RTC = buildRTC(Builder, NodeBuilder.getExprBuilder());
// original code is executed. BasicBlock *StartBlock = executeScopConditionally(S, this, RTC);
Value *V = NodeBuilder.getExprBuilder().create(AstInfo.getRunCondition());
Value *Zero = ConstantInt::get(V->getType(), 0);
V = Builder.CreateICmp(CmpInst::ICMP_NE, Zero, V);
BasicBlock *PrevBB = StartBlock->getUniquePredecessor();
BranchInst *Branch = dyn_cast<BranchInst>(PrevBB->getTerminator());
Branch->setCondition(V);
Builder.SetInsertPoint(StartBlock->begin()); Builder.SetInsertPoint(StartBlock->begin());
NodeBuilder.create(Ast); NodeBuilder.create(AI->getAst());
grosserUnsubmitted
Not Done
ReplyInline Actions

Very nice cleanup. This function is a lot more readable now.

grosser: Very nice cleanup. This function is a lot more readable now.
DEBUG(StartBlock->getParent()->dump());
grosserUnsubmitted
Not Done
ReplyInline Actions

Was adding this DEBUG statement intentional? I think it may be a little verbose for day-to-day use.

grosser: Was adding this DEBUG statement intentional? I think it may be a little verbose for day-to-day…
jdoerfertAuthorUnsubmitted
Not Done
ReplyInline Actions

I like to see the endresult of the code generation if I debug the whole thing, if I'm the only one then I can remove the stmt.

jdoerfert: I like to see the endresult of the code generation if I debug the whole thing, if I'm the only…
return true; return true;
} }
virtual void printScop(raw_ostream &OS) const {} virtual void printScop(raw_ostream &OS) const {}
virtual void getAnalysisUsage(AnalysisUsage &AU) const { virtual void getAnalysisUsage(AnalysisUsage &AU) const {
AU.addRequired<DominatorTreeWrapperPass>(); AU.addRequired<DominatorTreeWrapperPass>();
AU.addRequired<IslAstInfo>(); AU.addRequired<IslAstInfo>();
Show All 38 Lines