[GR-29230] Replace ValueMoveOp when rematerializing constants.

PullRequest: graal/8314
mttcr · Apr 20, 2021 · 2e2a9cf · 2e2a9cf
2 parents 5acbf8a + 94c2a4d
commit 2e2a9cf
Show file tree

Hide file tree

Showing 20 changed files with 87 additions and 99 deletions.
diff --git a/...ler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/Fields.java b/...ler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/Fields.java
@@ -302,38 +302,11 @@ public Class<?> getDeclaringClass(int index) {
      * @param value a value that will be assigned to the field
      */
     private boolean checkAssignableFrom(Object object, int index, Object value) {
-        assert value == null || getType(index).isAssignableFrom(value.getClass()) : String.format("Field %s.%s of type %s is not assignable from %s", object.getClass().getSimpleName(),
-                        getName(index), getType(index).getSimpleName(), value.getClass().getSimpleName());
-        return true;
-    }
-
-    public void set(Object object, int index, Object value) {
-        long offset = offsets[index];
-        Class<?> type = types[index];
-        if (type.isPrimitive()) {
-            if (type == Integer.TYPE) {
-                UNSAFE.putInt(object, offset, (Integer) value);
-            } else if (type == Long.TYPE) {
-                UNSAFE.putLong(object, offset, (Long) value);
-            } else if (type == Boolean.TYPE) {
-                UNSAFE.putBoolean(object, offset, (Boolean) value);
-            } else if (type == Float.TYPE) {
-                UNSAFE.putFloat(object, offset, (Float) value);
-            } else if (type == Double.TYPE) {
-                UNSAFE.putDouble(object, offset, (Double) value);
-            } else if (type == Short.TYPE) {
-                UNSAFE.putShort(object, offset, (Short) value);
-            } else if (type == Character.TYPE) {
-                UNSAFE.putChar(object, offset, (Character) value);
-            } else if (type == Byte.TYPE) {
-                UNSAFE.putByte(object, offset, (Byte) value);
-            } else {
-                assert false : "unhandled property type: " + type;
-            }
-        } else {
-            assert checkAssignableFrom(object, index, value);
-            UNSAFE.putObject(object, offset, value);
+        if (value != null && !getType(index).isAssignableFrom(value.getClass())) {
+            throw new GraalError(String.format("Field %s.%s of type %s in %s is not assignable from %s", object.getClass().getSimpleName(),
+                            getName(index), object, getType(index).getSimpleName(), value.getClass().getSimpleName()));
         }
+        return true;
     }
 
     public void setRawPrimitive(Object object, int index, long value) {
@@ -422,4 +395,9 @@ public void putObject(Object object, int i, Object value) {
         assert checkAssignableFrom(object, i, value);
         UNSAFE.putObject(object, offsets[i], value);
     }
+
+    public void putObjectChecked(Object object, int i, Object value) {
+        checkAssignableFrom(object, i, value);
+        UNSAFE.putObject(object, offsets[i], value);
+    }
 }
diff --git a/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/CompilationWrapper.java b/compiler/src/org.graalvm.compiler.core/src/org/graalvm/compiler/core/CompilationWrapper.java
@@ -33,6 +33,7 @@
 import static org.graalvm.compiler.debug.DebugOptions.Dump;
 import static org.graalvm.compiler.debug.DebugOptions.DumpPath;
 import static org.graalvm.compiler.debug.DebugOptions.MethodFilter;
+import static org.graalvm.compiler.debug.DebugOptions.PrintBackendCFG;
 
 import java.io.ByteArrayOutputStream;
 import java.io.File;
@@ -327,6 +328,7 @@ private T handleFailure(DebugContext initialDebug, Throwable cause) {
                             Dump, ":" + DebugOptions.DiagnoseDumpLevel.getValue(initialOptions),
                             MethodFilter, null,
                             DumpPath, dumpPath.getPath(),
+                            PrintBackendCFG, true,
                             TrackNodeSourcePosition, true);
 
             ByteArrayOutputStream logBaos = new ByteArrayOutputStream();

diff --git a/...org.graalvm.compiler.debug.test/src/org/graalvm/compiler/debug/test/DebugContextTest.java b/...org.graalvm.compiler.debug.test/src/org/graalvm/compiler/debug/test/DebugContextTest.java
@@ -67,7 +67,7 @@ static class DebugContextSetup {
             public List<DebugHandler> createHandlers(OptionValues options) {
                 return Arrays.asList(new DebugDumpHandler() {
                     @Override
-                    public void dump(DebugContext ignore, Object object, String format, Object... arguments) {
+                    public void dump(Object object, DebugContext ignore, boolean forced, String format, Object... arguments) {
                         dumpOutput.format("Dumping %s with label \"%s\"%n", object, String.format(format, arguments));
                     }
                 }, new DebugVerifyHandler() {

diff --git a/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugConfigImpl.java b/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugConfigImpl.java
@@ -268,7 +268,7 @@ public RuntimeException interceptException(DebugContext debug, Throwable e) {
                 if (!firstSeen.containsKey(o)) {
                     firstSeen.put(o, o);
                     if (DebugOptions.DumpOnError.getValue(options) || DebugOptions.Dump.getValue(options) != null) {
-                        debug.dump(DebugContext.BASIC_LEVEL, o, "Exception: %s", e);
+                        debug.forceDump(o, "Exception: %s", e);
                     }
                     debug.log("Context obj %s", o);
                 }

diff --git a/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugContext.java b/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugContext.java
@@ -1345,7 +1345,7 @@ public void forceDump(Object object, String format, Object... args) {
             closeAfterDump = true;
         }
         for (DebugDumpHandler dumpHandler : dumpHandlers) {
-            dumpHandler.dump(this, object, format, args);
+            dumpHandler.dump(object, this, true, format, args);
             if (closeAfterDump) {
                 dumpHandler.close();
             }

diff --git a/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugDumpHandler.java b/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/DebugDumpHandler.java
@@ -40,13 +40,14 @@ public interface DebugDumpHandler extends Closeable, DebugHandler {
      * If the type of {@code object} is supported by this dumper, then a representation of
      * {@code object} is sent to some consumer in a format determined by this object.
      *
-     * @param debug the debug context requesting the dump
      * @param object the object to be dumped
+     * @param debug the debug context requesting the dump
+     * @param forced true if called from {@link DebugContext#forceDump(Object, String, Object...)}
      * @param format a format string specifying a title that describes the context of the dump
      *            (e.g., the compiler phase in which request is made)
      * @param arguments arguments referenced by the format specifiers in {@code format}
      */
-    void dump(DebugContext debug, Object object, String format, Object... arguments);
+    void dump(Object object, DebugContext debug, boolean forced, String format, Object... arguments);
 
     /**
      * Flushes and releases resources managed by this dump handler. A subsequent call to

diff --git a/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/ScopeImpl.java b/compiler/src/org.graalvm.compiler.debug/src/org/graalvm/compiler/debug/ScopeImpl.java
@@ -220,7 +220,7 @@ public void dump(int dumpLevel, Object object, String formatString, Object... ar
             DebugConfig config = getConfig();
             if (config != null) {
                 for (DebugDumpHandler dumpHandler : config.dumpHandlers()) {
-                    dumpHandler.dump(owner, object, formatString, args);
+                    dumpHandler.dump(object, owner, false, formatString, args);
                 }
             }
         }

diff --git a/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Edges.java b/compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Edges.java
@@ -199,11 +199,6 @@ public void copy(Node fromNode, Node toNode) {
         }
     }
 
-    @Override
-    public void set(Object node, int index, Object value) {
-        throw new IllegalArgumentException("Cannot call set on " + this);
-    }
-
     /**
      * Sets the value of a given edge without notifying the new and old nodes on the other end of
      * the edge of the change.

diff --git a/...iler/src/org.graalvm.compiler.lir.amd64/src/org/graalvm/compiler/lir/amd64/AMD64Move.java b/...iler/src/org.graalvm.compiler.lir.amd64/src/org/graalvm/compiler/lir/amd64/AMD64Move.java
@@ -541,32 +541,30 @@ public static void move(CompilationResultBuilder crb, AMD64MacroAssembler masm,
         move((AMD64Kind) result.getPlatformKind(), crb, masm, result, input);
     }
 
-    public static void move(AMD64Kind moveKind, CompilationResultBuilder crb, AMD64MacroAssembler masm, Value result, Value input) {
+    private static void move(AMD64Kind moveKind, CompilationResultBuilder crb, AMD64MacroAssembler masm, Value result, Value input) {
         if (isRegister(input)) {
             if (isRegister(result)) {
                 reg2reg(moveKind, masm, result, input);
+                return;
             } else if (isStackSlot(result)) {
                 reg2stack(moveKind, crb, masm, result, asRegister(input));
-            } else {
-                throw GraalError.shouldNotReachHere("result=" + result + " result.class=" + result.getClass().getName());
+                return;
             }
         } else if (isStackSlot(input)) {
             if (isRegister(result)) {
                 stack2reg(moveKind, crb, masm, asRegister(result), input);
-            } else {
-                throw GraalError.shouldNotReachHere("result=" + result + " result.class=" + result.getClass().getName());
+                return;
             }
         } else if (isJavaConstant(input)) {
             if (isRegister(result)) {
                 const2reg(crb, masm, asRegister(result), asJavaConstant(input), moveKind);
+                return;
             } else if (isStackSlot(result)) {
                 const2stack(crb, masm, result, asJavaConstant(input));
-            } else {
-                throw GraalError.shouldNotReachHere("result=" + result + " result.class=" + result.getClass().getName());
+                return;
             }
-        } else {
-            throw GraalError.shouldNotReachHere("input=" + input + " input.class=" + input.getClass().getName());
         }
+        throw GraalError.shouldNotReachHere("input=" + input + " input.class=" + input.getClass().getName() + " " + "result=" + result + " result.class=" + result.getClass().getName());
     }
 
     private static void reg2reg(AMD64Kind kind, AMD64MacroAssembler masm, Value result, Value input) {

diff --git a/compiler/src/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/LIRIntrospection.java b/compiler/src/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/LIRIntrospection.java
@@ -103,15 +103,15 @@ protected Value getValue(Object obj, int index) {
         }
 
         protected void setValue(Object obj, int index, Value value) {
-            putObject(obj, index, value);
+            putObjectChecked(obj, index, value);
         }
 
         protected Value[] getValueArray(Object obj, int index) {
             return (Value[]) getObject(obj, index);
         }
 
         protected void setValueArray(Object obj, int index, Value[] valueArray) {
-            putObject(obj, index, valueArray);
+            putObjectChecked(obj, index, valueArray);
         }
     }
 

diff --git a/compiler/src/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/StandardOp.java b/compiler/src/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/StandardOp.java
@@ -50,7 +50,6 @@
 import jdk.vm.ci.code.StackSlot;
 import jdk.vm.ci.meta.AllocatableValue;
 import jdk.vm.ci.meta.Constant;
-import jdk.vm.ci.meta.JavaConstant;
 import jdk.vm.ci.meta.Value;
 
 /**
@@ -299,11 +298,6 @@ static boolean isLoadConstantOp(LIRInstruction op) {
             return op.isLoadConstantOp();
         }
 
-        default boolean canRematerialize() {
-            // By default only JavaConstants are assumed to be handled by the generic move
-            // operation.
-            return getConstant() instanceof JavaConstant;
-        }
     }
 
     /**

diff --git a/....compiler.lir/src/org/graalvm/compiler/lir/alloc/lsra/LinearScanAssignLocationsPhase.java b/....compiler.lir/src/org/graalvm/compiler/lir/alloc/lsra/LinearScanAssignLocationsPhase.java
@@ -26,10 +26,10 @@
 
 import static jdk.vm.ci.code.ValueUtil.isIllegal;
 import static jdk.vm.ci.code.ValueUtil.isRegister;
+import static org.graalvm.compiler.lir.LIRValueUtil.asVariable;
 import static org.graalvm.compiler.lir.LIRValueUtil.isCast;
 import static org.graalvm.compiler.lir.LIRValueUtil.isJavaConstant;
 import static org.graalvm.compiler.lir.LIRValueUtil.isStackSlotValue;
-import static org.graalvm.compiler.lir.LIRValueUtil.asVariable;
 import static org.graalvm.compiler.lir.LIRValueUtil.isVariable;
 import static org.graalvm.compiler.lir.LIRValueUtil.isVirtualStackSlot;
 
@@ -39,6 +39,7 @@
 
 import org.graalvm.compiler.core.common.cfg.AbstractBlockBase;
 import org.graalvm.compiler.debug.DebugContext;
+import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.debug.Indent;
 import org.graalvm.compiler.lir.CastValue;
 import org.graalvm.compiler.lir.ConstantValue;
@@ -169,9 +170,18 @@ private void assignLocations(ArrayList<LIRInstruction> instructions) {
                  * this can happen when spill-moves are removed in eliminateSpillMoves
                  */
                 hasDead = true;
-            } else if (assignLocations(op)) {
-                instructions.set(j, null);
-                hasDead = true;
+            } else {
+                try {
+                    LIRInstruction newOp = assignLocations(op);
+                    if (op != newOp) {
+                        instructions.set(j, newOp);
+                    }
+                    if (newOp == null) {
+                        hasDead = true;
+                    }
+                } catch (GraalError e) {
+                    throw e.addContext("lir instruction", "@" + op.id() + " " + op.getClass().getName() + " " + op);
+                }
             }
         }
 
@@ -211,11 +221,13 @@ public Value doValue(LIRInstruction instruction, Value value, OperandMode mode,
     /**
      * Assigns the operand of an {@link LIRInstruction}.
      *
-     * @param op The {@link LIRInstruction} that should be colored.
-     * @return {@code true} if the instruction should be deleted.
+     * @param inputOp The {@link LIRInstruction} that should be colored.
+     * @return the {@link LIRInstruction} that should be emitted. A {@code null} return deletes the
+     *         instruction.
      */
-    protected boolean assignLocations(LIRInstruction op) {
-        assert op != null;
+    protected LIRInstruction assignLocations(LIRInstruction inputOp) {
+        assert inputOp != null;
+        LIRInstruction op = inputOp;
 
         // remove useless moves
         if (MoveOp.isMoveOp(op)) {
@@ -226,7 +238,19 @@ protected boolean assignLocations(LIRInstruction op) {
                  * kicked out in LinearScanWalker.splitForSpilling(). When kicking out such an
                  * interval this move operation was already generated.
                  */
-                return true;
+                return null;
+            }
+        }
+
+        if (ValueMoveOp.isValueMoveOp(op)) {
+            ValueMoveOp valueMoveOp = ValueMoveOp.asValueMoveOp(op);
+            AllocatableValue input = valueMoveOp.getInput();
+            if (isVariable(input)) {
+                Value inputOperand = colorLirOperand(op, asVariable(input), OperandMode.USE);
+                if (inputOperand instanceof ConstantValue) {
+                    // Replace the ValueMoveOp with a constant materialization
+                    op = allocator.getSpillMoveFactory().createLoad(valueMoveOp.getResult(), ((ConstantValue) inputOperand).getConstant());
+                }
             }
         }
 
@@ -242,10 +266,10 @@ protected boolean assignLocations(LIRInstruction op) {
         if (ValueMoveOp.isValueMoveOp(op)) {
             ValueMoveOp move = ValueMoveOp.asValueMoveOp(op);
             if (move.getInput().equals(move.getResult())) {
-                return true;
+                return null;
             }
         }
-        return false;
+        return op;
     }
 
     @SuppressWarnings("try")

diff --git a/...compiler.lir/src/org/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase.java b/...compiler.lir/src/org/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase.java
@@ -28,8 +28,8 @@
 import static jdk.vm.ci.code.ValueUtil.asStackSlot;
 import static jdk.vm.ci.code.ValueUtil.isRegister;
 import static jdk.vm.ci.code.ValueUtil.isStackSlot;
-import static org.graalvm.compiler.lir.LIRValueUtil.isCast;
 import static org.graalvm.compiler.lir.LIRValueUtil.asVariable;
+import static org.graalvm.compiler.lir.LIRValueUtil.isCast;
 import static org.graalvm.compiler.lir.LIRValueUtil.isVariable;
 import static org.graalvm.compiler.lir.debug.LIRGenerationDebugContext.getSourceForOperandFromDebugContext;
 
@@ -905,9 +905,7 @@ protected void buildIntervals(boolean detailedAsserts) {
      * @param operand The destination operand of the instruction
      * @param interval The interval for this defined value.
      * @return Returns the value which is moved to the instruction and which can be reused at all
-     *         reload-locations in case the interval of this instruction is spilled. Currently this
-     *         can only be a {@link LoadConstantOp#canRematerialize() rematerializable constant
-     *         load}.
+     *         reload-locations in case the interval of this instruction is spilled.
      */
     protected Constant getMaterializedValue(LIRInstruction op, Value operand, Interval interval) {
         if (LoadConstantOp.isLoadConstantOp(op)) {
@@ -929,11 +927,7 @@ protected Constant getMaterializedValue(LIRInstruction op, Value operand, Interv
                     }
                 }
             }
-            Constant constant = move.getConstant();
-            if (!move.canRematerialize()) {
-                return null;
-            }
-            return constant;
+            return move.getConstant();
         }
         return null;
     }

diff --git a/...c/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/asm/CompilationResultBuilder.java b/...c/org.graalvm.compiler.lir/src/org/graalvm/compiler/lir/asm/CompilationResultBuilder.java
@@ -582,7 +582,7 @@ private void emitBlock(AbstractBlockBase<?> block) {
                     afterOp.accept(op);
                 }
             } catch (GraalError e) {
-                throw e.addContext("lir instruction", block + "@" + op.id() + " " + op.getClass().getName() + " " + op + "\n" + Arrays.toString(lir.codeEmittingOrder()));
+                throw e.addContext("lir instruction", block + "@" + op.id() + " " + op.getClass().getName() + " " + op);
             }
         }
     }

diff --git a/...graalvm.compiler.printer/src/org/graalvm/compiler/printer/BciBlockMappingDumpHandler.java b/...graalvm.compiler.printer/src/org/graalvm/compiler/printer/BciBlockMappingDumpHandler.java
@@ -58,7 +58,7 @@ public class BciBlockMappingDumpHandler implements DebugDumpHandler {
     private int nextId;
 
     @Override
-    public void dump(DebugContext debug, Object object, String format, Object... arguments) {
+    public void dump(Object object, DebugContext debug, boolean forced, String format, Object... arguments) {
         OptionValues options = debug.getOptions();
         if (object instanceof BciBlockMapping && DebugOptions.PrintGraph.getValue(options) != PrintGraphTarget.Disable) {
             try {