Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MatchContext should distinguish between nodes which are a pure function and side effecting ones #942

Open
tkrodriguez opened this issue Jan 28, 2019 · 1 comment

Comments

@tkrodriguez
Copy link
Member

commented Jan 28, 2019

MatchContext.validate tries to ensure that there isn't any unsafe work in between the nodes that are consumed as part of a match. The current definition is very conservative because we don't have a clear model of which nodes are unsafe to skip over. Looking at it from the other direction, for the nodes we match, we know their semantics and whether it's possible for other nodes to interact with them. Many of the nodes are pure functions so they can't be affected by intervening nodes. If we restrict the checked range to only visit from the root node to the last non-pure node then we could handle many more cases. I think we'd do this by adding a pureFunction element to the MatchableNode annotation.

Taking this as an example:

        1: (0) 81|Membar { barriers=0, stamp=void, location=ANY_LOCATION,  } pred={17|LoopBegin}
        2: (1) 120|Read { barrierType=NONE, stamp=i8 [0 - 1] ⇈0000000000000001, location=InfraControlL2.isDone, forceFixed=false, nullCheck=false,  } pred={81|Membar} address={122|AMD64Address}
        3: (0) 82|Membar { barriers=3, stamp=void, location=ANY_LOCATION,  } pred={120|Read}
        4: (1) 80|ZeroExtend { resultBits=32, inputBits=8, stamp=i32 [0 - 1] ⇈0000000000000001, getOp=org.graalvm.compiler.nodes.calc.ZeroExtendNode$$Lambda$15.897016085@7f61398cd358, inputAlwaysPositive=false, getReverseOp=org.graalvm.compiler.nodes.calc.ZeroExtendNode$$Lambda$16.1315998403@7f61398cd368,  } value={120|Read}

We would like to recognize that the ZeroExtend is a pure function so we don't have to check what nodes are between it and the Read, which would mean this is safe to match. The problem is that currently a match is emitted at the position of the last node matched because there might be other inputs that are only available at that point. So the other piece is figuring out the earliest point where all required inputs are available. So basically you walk back from endIndex examining each match node; if it's a pure function and it has no inputs outside of the match then you don't have to check it. Whatever that final index is becomes your emission point and the end of the region to be scanned for safety.

Then we need to fix MatchContext.setResult to put the emission on the right node for the emission point instead of on the root of the match.

@tkrodriguez

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

Looking at this a bit more, I think computing the emission point and the safety check region can become part of the matching itself. When building the match patterns with the annotation parser we can know whether the node we're operating on is pure and whether that node will end up taking inputs that are outside of the overall match. However, it might be more efficient to perform that check only once we've matched a pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.