[PATCH] For loop migration tool (round 1) - patch 2

High-level comment about "using std::string" and similar: this is not
done anywhere else in clang/llvm, so I'd suggest not to start doing
it...
I again have the feeling that not everything I'd like to be tested is
tested, but this time it's harder for me to put my finger on it :slight_smile:

+struct LoopFixerArgs {
+ tooling::Replacements *Replace;
+ DeclMap *GeneratedDecls;
+ ReplacedVarsMap *ReplacedVarRanges;
+};

What's the reason to not make those fields in LoopFixer?

+ for (const Stmt *Prev = Curr; Curr != NULL;
+ Prev = Curr, Curr = StmtParents->lookup(Prev))

It would seem simpler to me to write that as:
while(Curr != NULL) {
  ...
  Curr = StmtParents->lookup(Curr);
}

or, if you really prefer for loops (llvm isn't consistent here)
for (; Curr != NULL; Curr = StmtParent->lookup(Curr))

+ /// Run the analysis on Body, and return true iff the expression depends on
+ /// some variable declared within ContainingLoop.
+ bool isExternallyDependent(const Stmt *Body) {

I'm not sure I get the name: why is it "externally dependent" if it
depends on a variable within the loop? What does "externally" mean in
that context?

+ /// Attempts to find any usages of variables name Name in Body, returning
+ /// true when it is only used as an array index. A list of usages and
+ /// the arrays TargetVar indexes are also tracked.

Is that comment correct? From the implementation it looks like we
abort traversal when we find the first match.

+ DeclFinderASTVisitor(const std::string &Name,
+ const DeclMap *GeneratedDecls) :

Please document what GeneratedDecls is here...

+ friend class RecursiveASTVisitor<DependencyFinderASTVisitor>;

Why's that necessary?

+class DeclFinderASTVisitor : public RecursiveASTVisitor<DeclFinderASTVisitor> {
+ private:
+ const std::string &Name;

I'd strongly vote for making this a copy, as otherwise it's really
easy to introduce errors by handing in temps.

+// Generates the name to be used for an inserted iterator. It reliles on
+// declarationExists() to determine that there are no naming conflicts, and
+// tries to use some hints from the container name and the old index name.

Here's my next attempt!

High-level comment about "using std::string" and similar: this is not
done anywhere else in clang/llvm, so I'd suggest not to start doing
it...
I again have the feeling that not everything I'd like to be tested is
tested, but this time it's harder for me to put my finger on it :slight_smile:

That's partly because I hadn't included all the tests that could be added
at this point. They're in now.gg

+struct LoopFixerArgs {
+ tooling::Replacements *Replace;
+ DeclMap *GeneratedDecls;
+ ReplacedVarsMap *ReplacedVarRanges;
+};

What's the reason to not make those fields in LoopFixer?

The only reason apparent in this patch is to keep doConversion static
within LoopActions.cpp, though in another few patches I reuse this struct
across three LoopFixers (one per kind of convertible loop). Is there a
better way to arrange this? (LoopFixerArgs ends up with 10 parameters by
the end.)

+ for (const Stmt *Prev = Curr; Curr != NULL;
+ Prev = Curr, Curr = StmtParents->lookup(Prev))

It would seem simpler to me to write that as:
while(Curr != NULL) {
  ...
  Curr = StmtParents->lookup(Curr);
}

Agreed. Switched to a while loop.

or, if you really prefer for loops (llvm isn't consistent here)
for (; Curr != NULL; Curr = StmtParent->lookup(Curr))

+ /// Run the analysis on Body, and return true iff the expression
depends on
+ /// some variable declared within ContainingLoop.
+ bool isExternallyDependent(const Stmt *Body) {

I'm not sure I get the name: why is it "externally dependent" if it
depends on a variable within the loop? What does "externally" mean in
that context?

It's external to ContainingLoop. A full description would be "expression
has a free variable in the lexical context of the specified loop," which
would mean that hoisting Body outside of ContainingLoop would result in a
syntactic (variable not defined) or semantic (identifier refers to a
different variable) error. I'm going with dependsOnOutsideVariable for now.
Is hasFreeVariableInStmt a better name?

While I was at it, I also renamed ContainingLoop to ContainingStmt.

+ /// Attempts to find any usages of variables name Name in Body,
returning
+ /// true when it is only used as an array index. A list of usages and
+ /// the arrays TargetVar indexes are also tracked.

Is that comment correct? From the implementation it looks like we
abort traversal when we find the first match.

The comment was way off after the first line. Fixed.

+ DeclFinderASTVisitor(const std::string &Name,
+ const DeclMap *GeneratedDecls) :

Please document what GeneratedDecls is here...

It was somewhat opaque; comments added. That's what I use to track the
loop-to-generated-variable-name mapping.

+ friend class RecursiveASTVisitor<DependencyFinderASTVisitor>;

Why's that necessary?

CRTP + private overrides. This way, the VisitXXX functions don't have to be
public.

+class DeclFinderASTVisitor : public
RecursiveASTVisitor<DeclFinderASTVisitor> {
+ private:
+ const std::string &Name;

I'd strongly vote for making this a copy, as otherwise it's really
easy to introduce errors by handing in temps.

Done.

+// Generates the name to be used for an inserted iterator. It reliles on
+// declarationExists() to determine that there are no naming conflicts,
and
+// tries to use some hints from the container name and the old index name.
+
+string VariableNamer::createIndexName() {

s/reliles/relies/

Done.

Also, put the comment to the method.

Do you mean move the comment to the .h file? Done.

+bool VariableNamer::declarationExists(const string& Symbol) {
+ IdentifierInfo& Identifier = Context->Idents.get(Symbol);
+ DeclarationName Name =
+ Context->DeclarationNames.getIdentifier(&Identifier);
+
+ // First, let's check the parent context.
+ // FIXME: lookup() always returns the pair (NULL, NULL) because its
+ // StoredDeclsMap is not initialized (i.e. LookupPtr.getInt() is false
inside
+ // of DeclContext::lookup()). Why is this?
+ // NOTE: We work around this by checking when a shadowed declaration is
+ // referenced, rather than now.
+ for (const DeclContext *CurrContext = LoopContext; CurrContext != NULL;
+ CurrContext = CurrContext->getLookupParent()) {
+ DeclContext::lookup_const_result Result = CurrContext->lookup(Name);
+ if (Result.first != Result.second)
+ return true;
+ }
+
+ // Next, iterate through child contexts.

The above comment was incorrect. Fixed.

+ for (const Stmt *S = SourceStmt; S != NULL; S = ReverseAST->lookup(S)) {
+ DeclMap::const_iterator I = GeneratedDecls->find(S);
+ if (I != GeneratedDecls->end() && I->second == Symbol)
+ return true;
+ }
+ DeclFinderASTVisitor DeclFinder(Symbol, GeneratedDecls);
+ return DeclFinder.findUsages(SourceStmt);

Are all those return paths tested?

The first one doesn't fire on my tests due to the problem documented at the
top of this function. The others have tests in loop-convert-nesting.cpp and
loop-convert-naming.cpp (I didn't attach loop-convert-naming.cpp to the
original patch.), though they needed fixing due to a FileCheck quirk.

loop-converter-2.patch (32.8 KB)

+ // Common case shortcut: Expr's of different classes cannot be the same.
+ if (First->getStmtClass() != Second->getStmtClass())
+ return false;

Any reason to have the code in there? I'd rather go with "less code"
unless one of "more readable" or "really matters for performance" is
true...

+ /// Accessor for StmtAncestors.
+ const StmtMap &getStmtMap() {
+ return StmtAncestors;
+ }