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

[Sam Panzer]

Here's my next attempt!

On Mon, Aug 13, 2012 at 6:45 AM, Manuel Klimek <klimek at google.com> wrote:

>
> 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 :)
>

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.


>
>
> On Sat, Aug 4, 2012 at 1:14 AM, Sam Panzer <panzer at google.com> wrote:
> > Here are three patches which constitute most of the work needed for the
> > for-loop migration tool. The first contains the contents of the tutorial
> > from earlier this week, with extra comments and rebased into
> > clang/tools/extra/. The second patch fixes most of the correctness
> > assumptions that the naive tool makes (e.g. conflicting edits, name
> > conflicts), and the third adds some command-line flags along with a trick
> > for eliding a variable declared as a reference to the array access.
> >
> > Together, they provide a nearly complete converter for array-based loops
> -
> > the three optional features I didn't rebase into the patches involve
> using
> > an explicit type rather than auto, adding const when possible, and making
> > the loop variable a non-reference type for non-aggregate types if
> possible.
> >
> > There is one potentially problematic assumption that I haven't fixed yet:
> > the array expression is assumed not to change, as this becomes difficult
> to
> > verify when the code iterates over a compound expression such as
> > graph.nodes->getInputs(). In most cases, the loop will probably be
> > convertible if it passes all other checks, so I intend to offer a flag
> that
> > tells the loop converter whether or not to make these usually-okay
> changes.
> >
> > I also have two more patches in the works for next week, which add
> support
> > for iterator-based loops and loops over array-like containers.
> >
> > Thoughts?
> >
> > -Sam
> >
> >
> > _______________________________________________
> > cfe-dev mailing list
> > cfe-dev at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120813/334a4bc1/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: loop-converter-2.patch
Type: application/octet-stream
Size: 33637 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120813/334a4bc1/attachment.obj>