Wrong InputOutput field detection
Stagno opened this issue · 3 comments
As soon as one field is read and written dawn detects it as InputOutput
. However this isn't correct. Take as example:
nabla2 = ... #First time field nabla2 is referenced, it's a write
nabla2 = nabla2 + ... #Reads nabla2 which was written by the previous write within the same stencil
Clearly the value that field nabla2
had before running the stencil wouldn't matter, because it would be overwritten by the first write.
Thus dawn should classify it as Output
and not InputOutput
.
The detection algorithm should: classify as InputOutput
if a read exists which is not preceded by a write to the same field.
Classification happens here
dawn/dawn/src/dawn/IIR/AccessUtils.cpp
Line 37 in 6fdc460
and here
dawn/dawn/src/dawn/IIR/AccessUtils.cpp
Line 70 in 6fdc460
It is not checked whether there was a write before the read.
Unfortunately this issue turns out to be quite problematic.
- The computation you indicate is happening on the
DoMethod
. Since in unstructured we performPassStageSplitAllStatements
we don't have an opportunity to apply the criterion " classify asInputOutput
if a read exists which is not preceded by a write to the same field" in this place - We can thus either implement the intent computation as a Pass or as an update method of the IIR tree. The Pass solution requires non modifying the fields stored in the IIR tree from the outside, sacrificing some
const
-ness. - An implementation from within the IIR tree seems more appropriate. However
Stencil
,MultiStage
,Stage
andDoMethod
all hold local copies of their fields. Computing the intent at the stencil level requires to propagate the information down the tree (which is a pattern not well supported in dawn) - A field which may be
InputOutput
atStencil
scope may be justOutput
at a smaller scope. This means we either get different answers if we retrieve the Intend fromStencil
MultiStage
Stage
andDoMethod
or the information stored locally does not reflect the local intent. - What Intent does a temporary have? The caching pass has strong opinions on that. Also it seems that the Intend information at
DoMethod
Level is used to compute Read and WriteExtent
s. So the information Read/Write/ReadWrite and Input/Output/InputOutput is not clearly distinguished in dawn
A solution which does we want but is at least really close to a hack can be found here. In this version we compute the Intend information as suggested in the issue ot the Stencil
level, but leave the old computation on the DoMethod
Level in order to preserve the old caching and extent computation. This works out (tested with both stencils currently in the ICON-DSL repo), but really just works around dawn design issues in a really bad way.