Add declarations for variables reused in the reverse pass #688
Conversation
PetroZarytskyi
force-pushed
the
local-vars
branch
from
994fdd3 to
b9c1ace
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@PetroZarytskyi Can you please describe the solution in the commit message / pull request description? |
PetroZarytskyi
force-pushed
the
local-vars
branch
3 times, most recently
from
b54ec5f to
dea82f4
Compare
PetroZarytskyi
force-pushed
the
local-vars
branch
3 times, most recently
from
c2aa94c to
17d22e6
Compare
| cond = Visit(FS->getCond()); | ||
| const auto* IDRE = dyn_cast<DeclRefExpr>(FS->getInc()); | ||
| bool IDRE = isa<DeclRefExpr>(FS->getInc()); | ||
| /// FIXME: Why do we need to differentiate DeclRefExpr twice? |
There was a problem hiding this comment.
Which DeclRefExpr are you referring here?
There was a problem hiding this comment.
From what I get, inc is supposed to be the original increment (it's always differentiated later). Why is it differentiated specifically when it is a DeclRefExpr?
| StmtDiff IdxDiff = Visit(Indices[i]); | ||
| clonedIndices[i] = Clone(IdxDiff.getExpr()); |
There was a problem hiding this comment.
Is this change necessary for this pull request? If not, then this should be fixed in a separate pull-request.
There was a problem hiding this comment.
Well, it's necessary to replace getExpr with getRevSweepAsExpr for reverseIndices. I could move Clone to where they were previously if that matters.
test/Gradient/Loops.C
Outdated
| //CHECK-NEXT: clad::push(_t1, t); | ||
| //CHECK-NEXT: t *= x; | ||
| //CHECK-NEXT: { | ||
| //CHECK-NEXT: int i; |
There was a problem hiding this comment.
Is it necessary to declare the loop init statement outside the loop?
There was a problem hiding this comment.
The issue here is that we want to store the last value of the loop init. So we need to access i once right after the last condition. But we can't do that outside the loop since the variable is not declared there. My solution doesn't too look nice though so if you have a better idea feel free to share it.
PetroZarytskyi
force-pushed
the
local-vars
branch
3 times, most recently
from
9d152f6 to
c6961bf
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #688 +/- ##
==========================================
+ Coverage 94.54% 94.64% +0.09%
==========================================
Files 49 49
Lines 7189 7281 +92
==========================================
+ Hits 6797 6891 +94
+ Misses 392 390 -2
... and 1 file with indirect coverage changes
|
PetroZarytskyi
force-pushed
the
local-vars
branch
from
bcf4dfc to
44c754b
Compare
PetroZarytskyi
force-pushed
the
local-vars
branch
from
44c754b to
0b6373f
Compare
| // If the declaration is not located in the function global scope, | ||
| // we have to redeclare it in the reverse sweep since it won't be | ||
| // visible otherwise. | ||
| // FIXME: Is there a better way to determine non-function-global scope |
There was a problem hiding this comment.
Yes, there must be a better way for this. Have you tried VD->getDeclContext() != m_Function?
Ideally, function global scope variables should have the primal FunctionDecl as their DeclContext
There was a problem hiding this comment.
I'm not sure how DeclContext works exactly but getDeclContext seems to return m_Function for variables declared inside loops etc.
PetroZarytskyi
force-pushed
the
local-vars
branch
from
3ecbcf5 to
b388bec
Compare
|
The produced gradient of the below code has compile-time error in the local variable redeclaration part. double fn(double u, double v) {
double r;
{
double res;
double v_copy = v;
double &v_ref = v_copy;
res = v_ref;
r = res;
}
return r;
}The local variable redeclaration in the reverse sweep is as follows: double &v_ref = v_copy;
double v_copy = _t3;
double res = _t2;
|
| m_Blocks.pop_back(); | ||
| else | ||
| EmitReverseSweepDeclarations(); |
There was a problem hiding this comment.
Can we add a test case covering this branch?
There was a problem hiding this comment.
Currently, EndBlockWithoutCreatingCS is only used once in the reverse direction. There is no way to call it in the forward direction just with tests.
vgvassilev
force-pushed
the
local-vars
branch
from
a94a447 to
fa168e2
Compare
vgvassilev
force-pushed
the
local-vars
branch
from
fa168e2 to
1661a22
Compare
|
@PetroZarytskyi, is that PR superseded by #738? Can we close it? |
I think the solution implemented in #738 is better. We should probably close this PR. |
The problem this PR attempts to fix is described in #659 (which in turn caused #681). Also, this PR introduces placeholders to
DelayedGlobalStoreAndRefto rewrite expressions after they are added to the current block if there is no point in storing them. This is done to avoid cloning while handling multiplication differentiation.Fixes #659. Fixes #681.