[GR-24909] Ensure pointer identity for small int/long wrappers in compiled code.

PullRequest: graalpython/1160

Author: fangerer

graalpython/com.oracle.graal.python.test/src/tests/cpyext/test_misc.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # The Universal Permissive License (UPL), Version 1.0
@@ -194,6 +194,7 @@ def compile_module(self, name):
         lambda: (
             (True, lambda arg0, *args: arg0),
             (False, lambda arg0, *args: arg0),
+            (1000, lambda arg0, *args: arg0),
             (10, lambda arg0, *args: arg0),
             (10.0, lambda arg0, *args: arg0),
             (float('nan'), lambda arg0, *args: arg0),
@@ -256,7 +257,7 @@ def compile_module(self, name):
         ),
         code="""
         PyObject* wrap_PyEval_GetBuiltins() {
-            return Py_TYPE(PyEval_GetBuiltins());
+            return (PyObject *) Py_TYPE(PyEval_GetBuiltins());
         }
         """,
         resultspec="O",

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/CExtNodes.java

@@ -351,11 +351,25 @@ static PrimitiveNativeWrapper doIntegerSmall(@SuppressWarnings("unused") CExtCon
             return PrimitiveNativeWrapper.createInt(i);
         }
 
+        /**
+         * Generic integer conversion.<br/>
+         * In the interpreter: we just lookup the cached primitive native wrapper and use this
+         * instance. In compiled code, we do the same but create a fresh copy such that the compiler
+         * always sees a fresh instance. This avoids a phi and certainly a real allocation. Note: it
+         * is important to copy the existing cached wrapper because otherwise pointer equality is
+         * not ensured (see also:
+         * {@link AsPythonObjectBaseNode#mayUsePrimitive(IsPointerNode, PrimitiveNativeWrapper)}).
+         */
         @Specialization(replaces = "doIntegerSmall")
         static PrimitiveNativeWrapper doInteger(@SuppressWarnings("unused") CExtContext cextContext, int i,
                         @Shared("contextRef") @CachedContext(PythonLanguage.class) ContextReference<PythonContext> contextRef) {
-            if (CompilerDirectives.inInterpreter() && CApiGuards.isSmallInteger(i)) {
-                return doIntegerSmall(cextContext, i, contextRef);
+            if (CApiGuards.isSmallInteger(i)) {
+                if (CompilerDirectives.inInterpreter()) {
+                    return doIntegerSmall(cextContext, i, contextRef);
+                } else {
+                    // for explanation: see method doc
+                    return doIntegerSmall(cextContext, i, contextRef).copy();
+                }
             }
             return PrimitiveNativeWrapper.createInt(i);
         }
@@ -373,8 +387,13 @@ static PrimitiveNativeWrapper doLongSmall(@SuppressWarnings("unused") CExtContex
         @Specialization(replaces = "doLongSmall")
         static PrimitiveNativeWrapper doLong(@SuppressWarnings("unused") CExtContext cextContext, long l,
                         @Shared("contextRef") @CachedContext(PythonLanguage.class) ContextReference<PythonContext> contextRef) {
-            if (CompilerDirectives.inInterpreter() && CApiGuards.isSmallLong(l)) {
-                return doLongSmall(cextContext, l, contextRef);
+            if (CApiGuards.isSmallLong(l)) {
+                // for explanation of this construct: see 'ToSulongNode.doInteger'
+                if (CompilerDirectives.inInterpreter()) {
+                    return doLongSmall(cextContext, l, contextRef);
+                } else {
+                    return doLongSmall(cextContext, l, contextRef).copy();
+                }
             }
             return PrimitiveNativeWrapper.createLong(l);
         }
@@ -602,17 +621,27 @@ static PrimitiveNativeWrapper doLongSmall(@SuppressWarnings("unused") CExtContex
         @Specialization(replaces = "doIntegerSmall")
         static PrimitiveNativeWrapper doInteger(CExtContext cextContext, int i,
                         @Shared("contextRef") @CachedContext(PythonLanguage.class) ContextReference<PythonContext> contextRef) {
-            if (CompilerDirectives.inInterpreter() && CApiGuards.isSmallInteger(i)) {
-                return doIntegerSmall(cextContext, i, contextRef);
+            if (CApiGuards.isSmallInteger(i)) {
+                // for explanation of this construct: see 'ToSulongNode.doInteger'
+                if (CompilerDirectives.inInterpreter()) {
+                    return doIntegerSmall(cextContext, i, contextRef);
+                } else {
+                    return doIntegerSmall(cextContext, i, contextRef).copy();
+                }
             }
             return PrimitiveNativeWrapper.createInt(i);
         }
 
         @Specialization(replaces = "doLongSmall")
         static PrimitiveNativeWrapper doLong(CExtContext cextContext, long l,
                         @Shared("contextRef") @CachedContext(PythonLanguage.class) ContextReference<PythonContext> contextRef) {
-            if (CompilerDirectives.inInterpreter() && CApiGuards.isSmallLong(l)) {
-                return doLongSmall(cextContext, l, contextRef);
+            if (CApiGuards.isSmallLong(l)) {
+                // for explanation of this construct: see 'ToSulongNode.doInteger'
+                if (CompilerDirectives.inInterpreter()) {
+                    return doLongSmall(cextContext, l, contextRef);
+                } else {
+                    return doLongSmall(cextContext, l, contextRef).copy();
+                }
             }
             return PrimitiveNativeWrapper.createLong(l);
         }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/DynamicObjectNativeWrapper.java

@@ -1475,6 +1475,12 @@ private PrimitiveNativeWrapper(double dvalue) {
             this.dvalue = dvalue;
         }
 
+        private PrimitiveNativeWrapper(byte state, long value, double dvalue) {
+            this.state = state;
+            this.value = value;
+            this.dvalue = dvalue;
+        }
+
         public byte getState() {
             return state;
         }
@@ -1554,7 +1560,11 @@ public boolean equals(Object obj) {
 
         @Override
         public int hashCode() {
-            return (int) (value ^ Double.doubleToRawLongBits(dvalue) ^ state);
+            return (Long.hashCode(value) ^ Long.hashCode(Double.doubleToRawLongBits(dvalue)) ^ state);
+        }
+
+        PrimitiveNativeWrapper copy() {
+            return new PrimitiveNativeWrapper(state, value, dvalue);
         }
 
         @Override