Adding support for Cloning in the AST

Hi,

I am implementing the clone method and would like to contribute the implementation. Since this my first contribution, I would check if anybody have technical issues to highlight.

I also would like to check on some issues:

  1. I started out by making it virtual and provided default impl. in the base classes that just print the object class name. This is to allow me to test my clone methods before implementing it in all the AST classes. but finally it should be pure virtual.
  2. Should the clone return base class pointers or derived class pointers? For example currently, there is clone method implemented for literals in Expr.cpp. and it returns a pointer to the specifc literal type.
  3. I have implemented the clone method for different Stmts and Exprs and it seems working fine with exception of ImplicitCastExpr. Whenever I call clone on objects of these class I get the base class (Stmt) clone method invoked. I would appericiate if you could advise on this.

Code:
In the class declaration I add Stmt *Clone(ASTContext &C) const;

For the implementation:

Stmt BinaryOperator::Clone(ASTContext &C) const {
SourceLocation cpLoc;
Expr
lhs =dyn_cast(SubExprs[LHS]->Clone(C));
Expr* rhs =dyn_cast(SubExprs[RHS]->Clone(C));
BinaryOperator *cp= new (C) BinaryOperator(lhs, rhs , Opc, getType(), cpLoc);
return cp;
}

Stmt ArraySubscriptExpr::Clone(ASTContext &C) const {
SourceLocation cpLoc;
Expr
lhs =dyn_cast(SubExprs[LHS]->Clone(C));
Expr* rhs =dyn_cast(SubExprs[RHS]->Clone(C));
ArraySubscriptExpr *cp= new (C) ArraySubscriptExpr(lhs,rhs, getType(), cpLoc);
return cp;
}

Stmt *ImplicitCastExpr::Clone(ASTContext &C) const {
SourceLocation cpLoc;
ImplicitCastExpr *cp= new (C) ImplicitCastExpr(getType(), dyn_cast(getSubExpr()->Clone(C)), isLvalueCast());
return cp;
}

P.S: I have read the llvm developer policy.

Thanks,
Moataz

moataz ragab wrote:

Hi,

I am implementing the clone method and would like to contribute the
implementation. Since this my first contribution, I would check if
anybody have technical issues to highlight.

Be careful. Everyone who's working on template instantiation could be
implementing these as they go along. I'm about to commit cloners for
various literals (FloatingLiteral, CharacterLiteral, StringLiteral,
CXXBooleanLiteralExpr, CXXNullPtrExpr, GNUNullExpr, PredefinedExpr).

I also would like to check on some issues:
1. I started out by making it virtual and provided default impl. in
the base classes that just print the object class name. This is to
allow me to test my clone methods before implementing it in all the
AST classes. but finally it should be pure virtual.

Yes, it should.

2. Should the clone return base class pointers or derived class
pointers? For example currently, there is clone method implemented for
literals in Expr.cpp. and it returns a pointer to the specifc literal
type.

Always return the type you're cloning. Covariance is a wonderful thing.

3. I have implemented the clone method for different Stmts and Exprs
and it seems working fine with exception of ImplicitCastExpr. Whenever
I call clone on objects of these class I get the base class (Stmt)
clone method invoked. I would appericiate if you could advise on this.

Stmt *ImplicitCastExpr::Clone(ASTContext &C) const {
  SourceLocation cpLoc;
  ImplicitCastExpr *cp= new (C) ImplicitCastExpr(getType(),
dyn_cast<Expr>(getSubExpr()->Clone(C)), isLvalueCast());
  return cp;
}

I see nothing wrong with this. Perhaps if you sent a patch and an
example where we can try it out?

One comment: in the code above, you know that the result of
getSubExpr()->Clone() has to be an Expr. Use cast<> instead of
dyn_cast<> there.

Sebastian

Hi Sebastian,

Thanks.

Attached 3 files: the patch “clone.diff”. should be applied on the clang directory.
RewriteLoopUnroll.cpp you have to put it in the clang-cc directory.
test2.c: simple testcase for a forloop with array access in the body.

you can invoke the loopunroller using:

…/…/…/…/Debug/bin/clang-cc -loop-unroll tests/test2.c

Currently it crashes because ImplicitCastExpr object doesn’t get cloned. The Stmt base class gets called instead of ImplicitCastExpr

Best regards,
Moataz

clone.diff (10.6 KB)

RewriteLoopUnroll.cpp (8.97 KB)

test2.c (82 Bytes)

moataz ragab wrote:

Hi Sebastian,

Thanks.

Attached 3 files: the patch "clone.diff". should be applied on the
clang directory.
                         RewriteLoopUnroll.cpp you have to put it in
the clang-cc directory.
                         test2.c: simple testcase for a forloop with
array access in the body.

you can invoke the loopunroller using:

../../../../Debug/bin/clang-cc -loop-unroll tests/test2.c

Currently it crashes because ImplicitCastExpr object doesn't get
cloned. The Stmt base class gets called instead of ImplicitCastExpr

It calls ImplicitCastExpr::Clone just fine. It crashes because it can't
clone the VarDecl.

Sebastian

Sabastian,
You get different result than what I get. I get the debug message of the base class printed but not of the ImplicitCastExpr debug messages.

“calling the base class clone for the following: ImplicitCastExpr”

this confirms that it is not the code. it is something else.
Can you tell me which reversion you are using for llvm & Clang
I am using llvm & Clang reversion 71770. and gcc 4.3.2 20081105 (Red Hat 4.3.2-7)

What do you think the reason for this different output? How can I debug this?

Thanks,
Moataz

moataz ragab wrote:

Sabastian,
You get different result than what I get. I get the debug message of
the base class printed but not of the ImplicitCastExpr debug messages.
>> "calling the base class clone for the following: ImplicitCastExpr"

this confirms that it is not the code. it is something else.

Well, you could start by sending me the code you're actually using,
because the patch you sent doesn't contain anything to print debug messages.

Can you tell me which reversion you are using for llvm & Clang
I am using llvm & Clang reversion 71770. and gcc 4.3.2 20081105 (Red
Hat 4.3.2-7)

I'm at the 719xx level. GCC 4.3.3 Gentoo.

Sebastian

Why would we clone declarations?

   - Doug

Hi,

Attached is a new patch. I have made the debug messages more clear.
if you see **PASS: calling clone from ImplicitCastExpr then implicitExprCast clone was called successfully
if you see **FAIL: calling the base class clone for the following: … then it failed

For me I got the second message printed.

Thanks
Moataz

clone.diff (11 KB)

I am just testing the cloning by calling it recursively over the AST so it goes on. It should stop at the DeclRefExpr.

Whether it might be useful or not to clone a declaration, it might be useful if you want to create a new variable declaration similar to another declaration and only change the name.

Moataz

moataz ragab wrote:

Hi,

Attached is a new patch. I have made the debug messages more clear.
if you see **PASS: calling clone from ImplicitCastExpr then
implicitExprCast clone was called successfully
if you see **FAIL: calling the base class clone for the following:
.. then it failed

For me I got the second message printed.

The patch fails to apply to current trunk. Can you update and recreate,
please?

Sebastian

I am just testing the cloning by calling it recursively over the AST so it goes on. It should stop at the DeclRefExpr.

Okay.

Whether it might be useful or not to clone a declaration, it might be useful if you want to create a new variable declaration similar to another declaration and only change the name.

Hmmm, interesting. It seems to me like we really want to give the expression's Clone method some information that tells it what to do with declarations. For your loop-unrolling rewriter, we want to replace some variable declarations with other, similar variables (e.g., when the original variable was declared in the loop) or reuse the same variable declaration (if the variable is declared outside the loop). For my purposes, where I need to build a canonical expression, we replace types with their canonical types and replace some declarations (e.g., for non-type template parameters) with their "canonical" declarations. This tells me that we need to think about some kind of policy argument for Clone() that permits these changes.

   - Doug

Douglas Gregor wrote:

Whether it might be useful or not to clone a declaration, it might be
useful if you want to create a new variable declaration similar to
another declaration and only change the name.

Hmmm, interesting. It seems to me like we really want to give the
expression's Clone method some information that tells it what to do
with declarations. For your loop-unrolling rewriter, we want to
replace some variable declarations with other, similar variables
(e.g., when the original variable was declared in the loop) or reuse
the same variable declaration (if the variable is declared outside the
loop). For my purposes, where I need to build a canonical expression,
we replace types with their canonical types and replace some
declarations (e.g., for non-type template parameters) with their
"canonical" declarations. This tells me that we need to think about
some kind of policy argument for Clone() that permits these changes.

A callback to handle all sub-objects, instead of just cloning
recursively? It would be very tricky to do as a "policy" that is
anything less than a fully generic callback, given that cloning is deep.
The callback has to handle everything in that part of the AST somehow.
You can't stop at a single level.

Sebastian

Douglas Gregor wrote:

Whether it might be useful or not to clone a declaration, it might be
useful if you want to create a new variable declaration similar to
another declaration and only change the name.

Hmmm, interesting. It seems to me like we really want to give the
expression’s Clone method some information that tells it what to do
with declarations. For your loop-unrolling rewriter, we want to
replace some variable declarations with other, similar variables
(e.g., when the original variable was declared in the loop) or reuse
the same variable declaration (if the variable is declared outside the
loop).

For the loop unrolling rewriter, I was thinking about skipping the declarations in loop body and replace them with an assignmen if they were initializing the variable. I planned to stop the cloning at this level and make the expression point to the same declaration.
It doesn’t come to my mind a case where I would need to declare another variable. Please correct me if I am wrong?
But this is not a generic approach to cloning.

I had another application, where I want to do loop splitting on parallel processors and that will need to split the arrays to keep them local to each processor memory. I haven’t really gone through the implementation but was thinking of just cloning the declaration, change the name and size of the array.

For my purposes, where I need to build a canonical expression,

we replace types with their canonical types and replace some
declarations (e.g., for non-type template parameters) with their
“canonical” declarations. This tells me that we need to think about
some kind of policy argument for Clone() that permits these changes.

A callback to handle all sub-objects, instead of just cloning
recursively? It would be very tricky to do as a “policy” that is
anything less than a fully generic callback, given that cloning is deep.
The callback has to handle everything in that part of the AST somehow.
You can’t stop at a single level.

What do you think about passing a visitor to Clone()?

moataz ragab wrote:

    Douglas Gregor wrote:
    >
    >
    >> Whether it might be useful or not to clone a declaration, it
    might be
    >> useful if you want to create a new variable declaration similar to
    >> another declaration and only change the name.
    >
    >
    > Hmmm, interesting. It seems to me like we really want to give the
    > expression's Clone method some information that tells it what to do
    > with declarations. For your loop-unrolling rewriter, we want to
    > replace some variable declarations with other, similar variables
    > (e.g., when the original variable was declared in the loop) or reuse
    > the same variable declaration (if the variable is declared
    outside the
    > loop).

   For the loop unrolling rewriter, I was thinking about skipping the
declarations in loop body and replace them with an assignmen if they
were initializing the variable. I planned to stop the cloning at this
level and make the expression point to the same declaration.
It doesn't come to my mind a case where I would need to declare
another variable. Please correct me if I am wrong?

It's wrong in the case of C++ where initialization and assignment can
really differ. Sometimes one is possible, and the other isn't. The
simplest case is where the variable in question is a reference.

    A callback to handle all sub-objects, instead of just cloning
    recursively? It would be very tricky to do as a "policy" that is
    anything less than a fully generic callback, given that cloning is
    deep.
    The callback has to handle everything in that part of the AST somehow.
    You can't stop at a single level.

   What do you think about passing a visitor to Clone()?

It's probably what passing a callback comes down to.

Sebastian

I updated Clang and LLVM and guess what? it works well and the issue disappeared. I believe there was an issue and either it is solved or hidden. Attached the patch working with the latest revision for your reference.

Moataz

clone.diff (9.78 KB)

RewriteLoopUnroll.cpp (10.9 KB)