MeteoSwiss-APN/dawn

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

preField.setIntend(iir::Field::IntendKind::InputOutput);

and here
preField.setIntend(iir::Field::IntendKind::InputOutput);

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 perform PassStageSplitAllStatements we don't have an opportunity to apply the criterion " classify as InputOutput 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 and DoMethod 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 at Stencil scope may be just Output at a smaller scope. This means we either get different answers if we retrieve the Intend from Stencil MultiStage Stage and DoMethod 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 Write Extents. 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.

see more detailed discussion here: #1099