PSyclone icon indicating copy to clipboard operation
PSyclone copied to clipboard

OMP do-directive accepts non-loop node

Open hiker opened this issue 6 years ago • 2 comments

During #171 it was discovered that you can add a ProfileNode as child of a OMP-do-directive. This results in incorrect code (inside an omp-do loop must only be loops). In #171 I added a test to the ProfileTransform to prevent this from happening, but instead of adding this test to all potentially invalid child nodes, it should be added to the OMP do directive. Unfortunately this is not trivial to do, consider the following example:

 new_node.parent = directive_node
 directive_node.children.append(new_node)

If the second assignment would trigger an exception, we already have the directive node set as parent, but the directive would not have the new node as child - the graph is inconsistent :(

We would need to either:

  1. somehow enforce that first the child is added to the parent (which can then raise an exception) before adding the parent to the child - which is nearly impossible to guarantee
  2. add a specific moveNodeToNewParent() function, which would change both parent to child pointer and child to parent pointer, and would check that this is a valid operation - but this would require some significant refactoring of code. Though it might make some of the graph manipulations actually easier (atm it can easily happen that a small but messes up the graphs, with a function like the one above that would be less likely).

Not a very particular important bug (since the created code will not compile - that's at least better than creating incorrect code ;) ).

hiker avatar Jun 15 '18 14:06 hiker

A stop-gap solution might be to add a check at code-generation time that a Node has the correct type of children. I think I'll add this in as part of my OpenACC work (#170).

arporter avatar Jun 18 '18 10:06 arporter

I think we can do this check now using _update_node, and might be a good idea to do this earlier than later, and expand it to more types of nodes (since we have quite a few that have requirements on their child schedule now).

I drew up quick method that could do this for OMPDoDirective:

    def _update_node(self):
        '''
        Checks that the tree update hasn't invalidated the requirement
        that the only child of this directives schedule is a Loop.

        :raises GenerationError: If this node has more than
                                 one node in its schedule or that
                                 node is not a Loop.
        '''
        super()._update_node()
        # If we're still forming this node we can skip over these checks.
        if len(self.children) < 1:
            return
        if len(self.children[0].children) > 1:
            raise GenerationError(f"Found an OMPDoDirective with "
                                  f"{len(self.children[0].children)} "
                                  "nodes in its schedule. Expected 1.")
        if(len(self.children[0].children) != 0 and
           not isinstance(self.children[0].children[0], Loop)):
            raise GenerationError("Found a non-loop child of an "
                                  "OMPDoDirective.")

which would let us fail earlier - I'm not sure if there's any tree modifications that can change during update nodes that would prevent this from working? I think this is likely to be as early as PSyclone is to be able to fail without having specialisations of the Schedule node for each specific instance (e.g. creating nodes like class SingleLoopSchedule(Schedule) which would only accept a single child that is a Loop).

LonelyCat124 avatar Feb 22 '24 16:02 LonelyCat124