search

stfc / PSyclone

Domain-specific compiler and code transformation system for Finite Difference/Volume/Element Earth-system models in Fortran
BSD 3-Clause "New" or "Revised" License
103 stars 25 forks source link

transformations do not recognise typos in option names #613

Open rupertford opened 4 years ago

rupertford commented 4 years ago

If a transformation option is misspelt then the transformation silently returns. It would be better if unrecognised options were flagged as errors.

hiker commented 4 years ago

That is rather difficult to do (since it is unknown where an option might be used).

To start some brainstorming: 1) We introduce a kind of namespace: instead of 'check-node-type', we use 'ParallelTrans:check-node-type'. This way we could test if there is any invalid ParallelTrans option that is unknown ... but what if there is a typo in 'ParallelTras' :) ? And what if an option for transformations A1 and A2 is implemented in a common base-class B? Do we use namespace A1 (and A2), or B? Or do we change the namespace in A1 from A1 to B (and similar in A2 change the namespace from A2 to B)? That could get messy when we refactor things. 2) We could keep track of which option was used - it is a dictionary, so for each option 'x' after it was tested we add a key 'x-was-used' (or so). But then again, there is at least one place where I create a copy of the options dictionary (since I add options, and don't want to potentially modify the user's options directory), so the 'x-was-used' keys would not make it back to the user. Also, once we have added the 'x-was-used' keys, what if the same set of options are used several times ... then we would need a cleanup. 3) We change the options type from a dict to a special class, where we could automatically track accesses (calling options.is_set("bla") would mark 'bla' as being used). Nice, but a lot of work, without solving the issue that an option might not be used in certain call sequences. 4) We could have a separate 'validate_option' function, which would guarantee that all potentially used transformation will get called (while a validate of another transform might not be called if the transform is not used). That's the best option I could think of.

As a potential use case (not sure if this is realistic admittedly): I might have one transform that OMP-parallelises a subroutine by first renaming scalars (to make it easier to identify shared/private uses), then identifying parallel regions and then loops (I kind of have this, but ... no renaming for now, and it's a script, technically not a transform). So therefore I might have to disable node-type-check in the top level transformation, since the top-level is smart enough to only apply the transformation to the right nodes. But if the top level function decides that the subroutine can not be parallelised, so the omp transform will not be used, and the option for the transformation will not be tagged as 'used'. So we would get a warning then?

TBH, from an application user's point of view I don't see this as much of a problem: I would only apply an option if the default behaviour does not work ... and I will notice immediately if setting the option does not have the intended effect.

rupertford commented 4 years ago

How about all transformations provide a static list of valid keys and we have a validate_options function that gathers all valid keys then checks against them? Is that feasible? We could have a directory lookup path that could be extended in the config file (in case people wrote their own transformations outside of the system ones).