Allow explicit marking of local variables as the key for merge explosion with CompilerDirectives.mergeExplodeKey
#12557
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
gilles-duboscq
left a comment
gilles-duboscq
left a comment
There was a problem hiding this comment.
Please also check the 2 failed gates
truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/CompilerDirectives.java
Outdated
Show resolved
Hide resolved
truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/CompilerDirectives.java
Show resolved
Hide resolved
....graal.compiler/src/jdk/graal/compiler/truffle/substitutions/TruffleGraphBuilderPlugins.java
Show resolved
Hide resolved
| import jdk.graal.compiler.nodes.spi.CanonicalizerTool; | ||
|
|
||
| @NodeInfo(nameTemplate = "LoopExplosionKey", cycles = CYCLES_0, size = SIZE_0) | ||
| public final class LoopExplosionKeyNode extends FloatingNode implements Canonicalizable { |
There was a problem hiding this comment.
What happens if the directive is used in a non-merge-explode method and this node remains in the graph?
I think this should either be a very explicit compilation failure or performance warning. If it's only a warning then we need to remove such nodes.
@chumer what do you think? Should using CompilationDirective.mergeExplodeKey outside of the context of a @ExplodeLoop(kind = LoopExplosionKind.MERGE_EXPLODE) method be a compilation error or a performance warning?
There was a problem hiding this comment.
I would go with a compilation error. We can do a similar thing as in VerifyFrameDoesNotEscapePhase.
Register the phase in TruffleTier.
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Show resolved
Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Outdated
Show resolved
Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Outdated
Show resolved
Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Show resolved
Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Outdated
Show resolved
Hide resolved
...esso/src/com.oracle.truffle.espresso/src/com/oracle/truffle/espresso/nodes/BytecodeNode.java
Show resolved
Hide resolved
blaumeise20
force-pushed
the
alra/merge-explode-key
branch
from
d9701be to
911f48b
Compare
| * @param i the variable that should be used as the merge key. | ||
| * @return the unchanged value | ||
| * | ||
| * @since 25.1 |
There was a problem hiding this comment.
Please add an entry about this in the truffle changelog.md. Including a migration recommendation that if you use ExplodeLoop MERGE that you should also use this new API.
There was a problem hiding this comment.
What issue ID should I specify?
truffle/src/com.oracle.truffle.api/src/com/oracle/truffle/api/CompilerDirectives.java
Show resolved
Hide resolved
| * lose the node when encoding the graph. At the time of decoding, | ||
| * `canDelayIntrinsification` will be false, so the actual node is created. | ||
| */ | ||
| return false; |
There was a problem hiding this comment.
That is a good thought, but I don't think it is necessary here to delay the intrinisification.
We delay it for isPartialEvaluationConstant because the interpretation will be different if the argument is a constant and it is only really a PE constant during decoding. For this intrinsic it does not make a difference as we unconditionally wrap the value into LoopExplosionKeyNode. So I think it would actually be better always do this during parsing already, as a method once parsed is decoded many times, so we avoid the overhead of applying the intrinsic here. Probably a minor effect on compile speed, but small things add up and its also simpler this way.
There was a problem hiding this comment.
I couldn't manage to keep the node alive until decoding. It would always vanish, even though I explicitly made canonicalize return this. This was the only solution I found that worked. Do you have any idea why the node could have been deleted?
| import jdk.graal.compiler.nodes.spi.CanonicalizerTool; | ||
|
|
||
| @NodeInfo(nameTemplate = "LoopExplosionKey", cycles = CYCLES_0, size = SIZE_0) | ||
| public final class LoopExplosionKeyNode extends FloatingNode implements Canonicalizable { |
There was a problem hiding this comment.
I would go with a compilation error. We can do a similar thing as in VerifyFrameDoesNotEscapePhase.
Register the phase in TruffleTier.
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Show resolved
Hide resolved
compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphDecoder.java
Show resolved
Hide resolved
3 participants
This PR adds a method
CompilerDirectives.mergeExplodeKeywhich can be used to explicitly mark variables to be used as the merge key inMERGE_EXPLODEloop explosion.Currently, the entire frame state is used for merging. Taking Espresso as an example, if you would jump to the same
bcibut assign a different value to another variable, it will result in two separate code paths. This can lead to unexpected graph size explosion and bugs. With the method introduced in this PR, you can mark a single variable to be used as the key. Truffle will merge on this key only, but verify that other variables stay the same. If merging should occur but there is a mismatch, the compilation will bail out.Future steps include allowing multiple variables to be marked simultaneously (which has to be extended into irreducible loop handling), and creating an option for explicit PHI nodes, so local variables can be used as counters without the need for objects.