Wrong derivation of runtime parameters

[mlange05]

The property Operator.parameters, which defines the list of arguments that needs to be supplied to or derived by the operator at runtime, includes various Dimension-related variables that are not actually required by the generated kernel code. As a result, the auto-derivation and the "Arguments Engine" are overly complex and introduce a lot of potentially redundant complexity to rectify this "wrong spec". I believe a much more prudent approach would be to correct the op.parameters derivation based on the final IR, instead of trying to estimate the required variables from the list of Dimension objects as we do currently.

As an example, the simple kernel generated by the following snippet requires only the "start" and "end" values for the time loop (t_s/t_e and time_e/time_s following PR #453):

from devito import *
grid = Grid(shape=(5, 6))
f = TimeFunction(name='f', grid=grid)
op = Operator(Eq(f, f + 3))

However, in both master and #453 op.parameters contains all of [t_s, t_e, t_size, time_s, time_e, time_size], thus forcing some pretty complex information flow during the auto-derivation, despite the variables never being used.

Potential solution:

Instead of adding all variables dim_s, dim_e, dim_size for each Dimension dim, I believe we should derive the individual variables in op.parameters from the final IR AST (op.nodes). For this to work we need to ensure that all dimension-related variables are symbols that are reachable using the symbolic search routines (integer-based Constants maybe?). This is tricky, however, as neither the variable casts that require the dim_size variables, nor the loop headers that need dim_s/dim_e are currently being traversed by things like the FindSymbols visitor.

Using this approach, the perceived/pseudo-cyclic dependency between derived and parent Dimensions (eg. t and time) could then be avoided; either by ignoring DerivedDimension values and always deferring to the parent, or by auto-detecting if a symbol is defined within a particular kernel, or needs to be defined externally via arguments (more complex, but maybe "cleaner").

In addition, I believe an IR-based derivation of parameters would also help integrate the DLE and AT-specific arguments more neatly, as the variables introduced by BlockingDimensions now just become additional variables that are inserted into the IR/AST by the DLE picked up automatically.

Future improvements

If the described idiosyncrasy were removed, I believe large swathes of the "argument engine" code, including the generic visitor pattern introduced in #453, become either redundant or can at least be simplified significantly, because the rules for argument defaulting and user-overrides then become fairly linear and straightforward (objectively). I have a branch to roughly demonstrate this (mlange-arguments) and/or I can raise a separate issue with a (again objectively) "neater" design concept if this is of use to anyone. However, I regard the problem described above as a separate "bug" and I would argue that continuing to work around it instead of fixing it will become excessively laborious and detrimental to code quality (personal opinion, of course).

[FabioLuporini]

@navjotk I think this discussion would strongly benefit from your input.

[navjotk]

I'm actually not sure what problem you are talking about here(there is definitely more than one). Could I summarise the issue as "we have too many arguments"?

I do agree with the general idea of IR based arguments derivation. I was trying to move towards that with the recent rewrite.

How reducing the number of arguments makes deriving values simpler is unclear to me at this point though.

I have seen your branch and I like the use of MultiDict. This would have been very useful if I knew something like this existed.

[mlange05]

I'm actually not sure what problem you are talking about here(there is definitely more than one). Could I summarise the issue as "we have too many arguments"?

Yes, to be more precise, we should only have arguments in .parameter that are actually used in the code.

I do agree with the general idea of IR based arguments derivation. I was trying to move towards that with the recent rewrite.

Agreed. The argument type hierarchy before #453 made this near impossible, and my latest experimentation with this should slot in near cleanly post-rewrite (better-arguments, still a prototype though).

How reducing the number of arguments makes deriving values simpler is unclear to me at this point though.

Consider two use cases:

• A kernel with both time and t in the code (eg. acoustic) and time_e and time_s given by the user during invocation. If all of [t_e, t_s, time_e, time_s] are in .parameters, we need to derive the value for t_e from time_e (and similar for t_s <- time_s).
• A kernel with only t_e and t_s (eg Eq(f, f + 1)) needs to derive values for time_s/time_e from the t_s/t_e.

In those two cases the information flow we are trying to encode is the complete opposite of each other, and only necessary due to the presence of all time-related start/end parameters. If, in contrast we were to treat these variables from the IR-AST, we would infer that t_e/t_s is always defined in terms of the parent as part of the loop header, so we only ever need to auto-derive time_e/time_s and we only need to encode a single-direction flow of information.

A similar argument can be made for time_size, which is never used in any real test cases, since the cast only uses TimeFunction.symbolic_shape[1:] (ie. only the shape dimension sizes).

[navjotk]

In the ideal scenario, _size arguments should not be accepted from the user. There is never a need for the user to directly tell us the size of an array.

Also, how about removing all derived dimensions from arguments? That solves both the issues you raise without a rewrite.

[mlange05]

In the ideal scenario, _size arguments should not be accepted from the user. There is never a need for the user to directly tell us the size of an array.

For t_size I'm not talking about user overrides, but the derivation of default values from other parameters. For those, it should almost never be necessary to derive t_size (assuming save=False), while x_size/y_size is usually needed for a regular TimeDimension object f(t, x, y,).

Also, how about removing all derived dimensions from arguments? That solves both the issues you raise without a rewrite.

From the parameters, yes, that's what the aliasing or pass-through approach would do naturally. For user overrides, however, I would like derived dimensions to alias their parents, for the simple reason that for the trivial kernel Eq(f, f + 1) with a TimeFunction f and save=False, the symbol time that is now strictly required by the kernel is not even in the expression. I'd argue that this would be rather confusing to users and counter-intuitive to strictly require op.apply(time=<something>) for the above example.

[mlange05]

I do agree with the general idea of IR based arguments derivation. I was trying to move towards that with the recent rewrite.

Agreed. The argument type hierarchy before #453 made this near impossible, and my latest experimentation with this should slot in near cleanly post-rewrite (better-arguments, still a prototype though).

Alas, I was wrong - it looks like this will not slot in as neatly as I had hoped. The problem is that the current argument derivation makes a bunch of restrictive assumptions about the .parameters list, like actual Dimension types being included, as well as some ordering constraints. Since these are hard to adhere to with a generic parameter derivation, it looks like the we'll have to do both at the same time.