Robert Bradshaw, 28.01.2011 22:49: > On Fri, Jan 28, 2011 at 1:01 PM, Stefan Behnel wrote: >> Stefan Behnel, 27.01.2011 09:40: >>> Robert Bradshaw, 27.01.2011 07:39: >>>> On Wed, Jan 26, 2011 at 10:29 PM, Stefan Behnel wrote: >>>>> there is this code in Sage (wrapper_rr.pyx/.pxd): >>>>> >>>>> cdef class Wrapper_rr(Wrapper): >>>>> cdef int _n_args >>>>> cdef mpfr_t* _args >>>>> ... >>>>> >>>>> # offending line: >>>>> mpfr_init2(self._args[i], self.domain.prec()) >>>>> >>>>> The signature of mpfr_init2() is >>>>> >>>>> ctypedef __mpfr_struct* mpfr_t >>>>> void mpfr_init2 (mpfr_t x, mp_prec_t prec) >>>>> [...] >>>>> sage/ext/interpreters/wrapper_rr.c:2737: error: incompatible types in >>>>> assignment >>>>> >>>>> Any idea what might be going wrong here? >>>> >>>> mpfr_t is actually an __mpfr_struct[1]. (It's a clever hack to have >>>> stack-allocated, pass-by-reference semantics.) We only need to pull >>>> non-simple arguments into temp variables. >>> >>> Ah, that would explain it. But this also means that there may be cases >>> where we can't help breaking existing code with such a change. >> >> Ok, this particular code really can't be worked around by Cython. From the >> POV of the compiler, the second function argument (a Python function call >> result) may potentially have side effects on the first one, so both must be >> coerced to a temp in the right order to be correct. >> >> 2732 /* "sage/ext/interpreters/wrapper_rr.pyx":56 >> 2733 * if self._args == NULL: raise MemoryError >> 2734 * for i in range(count): >> 2735 * mpfr_init2(self._args[i], self.domain.prec()) #<<<<< >> 2736 * val = args['constants'] >> 2737 * self._n_constants = len(val) >> 2738 */ >> >> 2739 __pyx_t_8 = (((struct >> __pyx_obj_4sage_3ext_12interpreters_10wrapper_rr_Wrapper_rr >> *)__pyx_v_self)->_args[__pyx_v_i]); >> >> 2740 __pyx_t_2 = PyObject_GetAttr(((PyObject *)((struct >> __pyx_obj_4sage_3ext_12interpreters_10wrapper_rr_Wrapper_rr >> *)__pyx_v_self)->domain), __pyx_n_s__prec); if /*...*/ >> >> 2741 __Pyx_GOTREF(__pyx_t_2); >> 2742 __pyx_t_3 = PyObject_Call(__pyx_t_2, ((PyObject >> *)__pyx_empty_tuple), NULL); if /*...*/ >> >> 2743 __Pyx_GOTREF(__pyx_t_3); >> 2744 __Pyx_DECREF(__pyx_t_2); __pyx_t_2 = 0; >> 2745 __pyx_t_9 = __Pyx_PyInt_from_py_mp_prec_t(__pyx_t_3); if /*...*/ >> 2746 __Pyx_DECREF(__pyx_t_3); __pyx_t_3 = 0; >> 2747 mpfr_init2(__pyx_t_8, __pyx_t_9); >> >> >> And given that Cython cannot know that the pointer is actually not a >> pointer, it generates the expected code here which only the C compiler can >> detect as invalid. >> This needs fixing in Sage in one way or another. > > Cython didn't used to support tyepdef'd arrays, so that's why it was > declared as a pointer, but yes, that should be fixed. I'll do that in > our copy, and it will go out with the 0.14.1 fixes (even though we > don't need it yet). There's still the case of, say, > > mpfr_init2(self._args[f(i)], self.domain.prec()) > > where f is a cdef int -> int function. Do we handle that?
Sure. The problem isn't (any longer) that we *forget* to put things into a temp, the problem is that we try to put things into a temp now that we cannot store away. I've been dropping some cases in my latest commits where the temp copying mechanism was triggered needlessly. It's basically a matter of properly implementing is_simple() for an ExprNode in order to decide when a non-temp result is side-effect free. I'm still not sure if a coerce_to_owned() method would be worthwhile. As long as SimpleCallNode is the only place where this is actually needed, it's fine to leave the code there. Can anyone think of other places where the order of C value evaluation matters? Tuples and other container literals may qualify, but given that they use only Python values, I don't see it being a problem there. Stefan _______________________________________________ Cython-dev mailing list Cython-dev@codespeak.net http://codespeak.net/mailman/listinfo/cython-dev
