spcl / dace

DaCe - Data Centric Parallel Programming
http://dace.is/fast
BSD 3-Clause "New" or "Revised" License
499 stars 129 forks source link

[Multi SDFG branch, Fortran frontend] Several `par_Decl_Range_Finder()` were missing the `structure` argument (therefore passing a wrong boolean argument in its place). #1752

Closed pratyai closed 1 week ago

pratyai commented 1 week ago

Additionally:

mcopik commented 1 week ago

@pratyai, I'm not sure I follow - can you show me a simple test case of Fortran where we previously failed?

pratyai commented 1 week ago

@pratyai, I'm not sure I follow - can you show me a simple test case of Fortran where we previously failed?

Failure for par_Decl_Range_Finder()? I can show none, however please notice that par_Decl_Range_Finder() has the following signature (I've annotated by their order):

def par_Decl_Range_Finder(node: ast_internal_classes.Array_Subscript_Node,  # 0
                          ranges: list,  # 1
                          rangepos: list,  # 2
                          rangeslen: list,  # 3
                          count: int,  # 4
                          newbody: list,  # 5
                          scope_vars: ScopeVarsDeclarations,  # 6
                          structures: Structures,  # 7
                          declaration=True):  # 8 (kw)
...definition here...

Now, notice the following call (one of the several updated in this changelist; I've broken it up into multiple lines for ease of tracking):

par_Decl_Range_Finder(self.first_array,  # 0
  self.loop_ranges,  # 1
  [],  # 2
  rangeslen_left,  # 3
  self.count,  # 4
  new_func_body,  # 5
  self.scope_vars,  # 6
  True)  # 7 (??? This is the positional argument `structures: Structures`)

My understanding is that these calls are so clearly wrong that they don't even call for a test case. While it is possible that structures is actually a boolean (or an union of boolean and other types), I thought that's unlikely. If that happens to be the case, we should then update the type-hint (and possibly the argument name too).

pratyai commented 1 week ago

For the assert hasattr(node, "_fields") part, I just now realize that node is an FNode, so it is always true. I'll be happy to drop that line.

As for the assert i.type in {'ALL', 'RANGE'} line, I can also take it out. However, IMO we could use Enum types for the string types that have a small number of known possible values (e.g., here and also the data types). It is difficult to track down what a string's value is supposed to be when they are all just plain str.