[cfe-dev] [PATCH] For loop migration tool (round 1) - patch 2
Manuel Klimek
klimek at google.com
Thu Aug 16 10:55:40 PDT 2012
+ // 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;
+ }
+
+ /// Accessor for DeclParents.
+ const DeclParentMap &getDeclMap() {
+ return DeclParents;
+ }
Please document from what to what those maps map. Also, perhaps change
the name of the methods to make that more obvious:
getStmtToParentStmtMap()
getDeclToParentDeclStmtMap()
or something.
+// Determine if any this variable is declared inside the ContainingStmt.
s/any//
+ DependencyFinderASTVisitor DependencyFinder(&ParentFinder->getStmtMap(),
+ &ParentFinder->getDeclMap(),
+ Args->ReplacedVarRanges,
+ FS);
If FS were not called FS, I might not have to open a different patch
to actually find out what it means :( (/me is generally grumpy about
TLAs - not your fault, they're all over clang). My brain just can't
induce them from context.
+/// Class used to determine if an expression is dependent on a
variable declared
+/// inside of the loop where it would be used.
+class DependencyFinderASTVisitor :
+ public RecursiveASTVisitor<DependencyFinderASTVisitor> {
How this works is still a riddle to me. Can you give me an example of
what this is trying to protect against?
+// If we already created a variable for FS, check to make sure it has a
+// different name.
+bool DeclFinderASTVisitor::VisitForStmt(ForStmt *FS) {
I think the second part of that comment is confusing, as the code
doesn't actually select a different name, it merely sets "Found".
+ void findExprComponents(const Expr *SourceExpr) {
+ Expr *E = const_cast<Expr *>(SourceExpr);
+ RecursiveASTVisitor<ComponentFinderASTVisitor>::TraverseStmt(E);
Any reason for casting and then calling TraverseStmt?
+bool DeclFinderASTVisitor::VisitDeclRefExpr(DeclRefExpr *DRE) {
+ if (NamedDecl *ND = dyn_cast_or_null<NamedDecl>(DRE->getDecl()))
Do we get calls of VisitDeclRefExpr with NULL? That would be surprising...
Not yet fully through with the review, sending out what I have in the
meantime...
Cheers,
/Manuel
On Tue, Aug 14, 2012 at 3:21 AM, Sam Panzer <panzer at google.com> wrote:
>
> 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.)
Now I really want more context in the patch :( (textual context, that is).
Perhaps all we need is a slightly better name, but when you foresee 10
parameters, I'll only see that when I see the end state of the code.
Perhaps what really makes my spidey sense tingle is a struct that
doesn't have a constructor to ensure that all fields must be set (and
you can't forget that where you construct it). I personally prefer
constructors for that.
>> + 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.
Ah, ok, makes sense, as the visitors here are not purely an
implementation detail...
>> +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
Then I'd put the code in as a comment on what we'd like to do, and
what doesn't work; I would strongly advise not to have code in that we
know doesn't work. Or am I misunderstanding something?
> 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
>> >
>
>
More information about the cfe-dev
mailing list