Problem adding match rule for (ReadNode address)

[Doug Simon]

Hi Andrew,

> On 30 Jan 2017, at 14:03, Andrew Dinn <adinn at redhat.com> wrote:
> 
> I have been trying to add a match rule for a ReadNode to the AArch64
> graal LIRGenerator and was foiled by the following bug (which has been
> accidentally side-stepped by the x86 implementation -- details below).
> 
> If I add a match rule/handler method to the end of class
> AArch64NodeMatchRules as follows
> 
> graal/org.graalvm.compiler.core.aarch64/src/org/graalvm/compiler/core/aarch64/AArch64NodeMatchRules.java
> @@ -33,6 +33,8 @@ import org.graalvm.compiler.nodes.DeoptimizingNode;
> import org.graalvm.compiler.nodes.calc.SignExtendNode;
> import org.graalvm.compiler.nodes.calc.ZeroExtendNode;
> import org.graalvm.compiler.nodes.memory.Access;
> +import org.graalvm.compiler.nodes.memory.ReadNode;
> +import org.graalvm.compiler.nodes.ValueNode;
> 
> import jdk.vm.ci.aarch64.AArch64Kind;
> 
> @@ -75,4 +77,9 @@ public class AArch64NodeMatchRules extends
> NodeMatchRules {
>         AArch64Kind memoryKind = getMemoryKind(access);
>         return builder ->
> getArithmeticLIRGenerator().emitExtendMemory(true, memoryKind,
> root.getResultBits(), (AArch64AddressValue)
> operand(access.getAddress()), getState(access));
>     }
> +
> +    @MatchRule("(Read address)")
> +    public ComplexMatchResult checkRead(ReadNode root, ValueNode address) {
> +        return null;
> +    }
> }
> 
> then the build process fails as follows.
> 
> Compiling org.graalvm.compiler.replacements.amd64 with javac-daemon...
> [dependency GRAAL_NODEINFO_PROCESSOR updated]
> /home/adinn/openjdk/graal/graal-core/mxbuild/graal/org.graalvm.compiler.core.aarch64/src_gen/org/graalvm/compiler/core/aarch64/AArch64NodeMatchRules_MatchStatementSet.java:23:
> error: cannot find symbol
>            return ((AArch64NodeMatchRules)
> nodeMatchRules).checkRead((ReadNode) args[0], (ValueNode) args[1]);
> 
>                  ^
>  symbol:   class ValueNode
>  location: class MatchGenerator_checkRead
> 1 error
> 
> The error happens because the generated matching engine code in
> AArch64NodeMatchRules_MatchStatementSet.java does not include an import
> declaration for class ValueNode. Imports are only generated for the
> packages to which root nodes of matched terms and subterms belong but
> not for packages containing Value arguments like address.
> 
> Note that the corresponding x86 code successfully employs match rules
> which refer to ValueNode arguments because it includes rules (e.g. the
> ones for IfNode) whose root nodes lie in the same package as ValueNode.
> 
> The fix is to modify class MatchProcessor so it always generates an
> import for ValueNode
> 
> +++
> b/graal/org.graalvm.compiler.core.match.processor/src/org/graalvm/compiler/core/match/processor/MatchProcessor.java
> @@ -521,6 +521,7 @@ public class MatchProcessor extends AbstractProcessor {
>             out.println("import " + NodeMatchRules.class.getName() + ";");
>             out.println("import " + Position.class.getName() + ";");
>             out.println("import " + ServiceProvider.class.getName() + ";");
> +            out.println("import " + ValueNode.class.getName() + ";");
>             for (String p : info.requiredPackages) {
>                 out.println("import " + p + ".*;");
>             }
> 
> Do you need this patch to be submitted via a github pull request? Do I
> need to raise an associated issue?

Thanks for finding and reporting the bug, complete with a fix.

For such a small fix, it’s easiest for you to raise an issue on github instead of creating a pull request. Once you’ve opened the github issue, I’ll assign it to someone.

-Doug