Opened 6 years ago

Closed 3 months ago

#77 closed defect (fixed)

temp allocation should be done in code generation phase

Reported by: robertwb Owned by: somebody
Priority: major Milestone: wishlist
Component: Code Generation Keywords:
Cc:

Description

This is a follow up to #67.

Change History (6)

comment:1 Changed 6 years ago by dagss

Started this here:

http://hg.cython.org/cython-dagss/rev/33d322bd7b32

If this works, it should allow for painless, incremental migration of the temps.

comment:2 Changed 6 years ago by dagss

It has a problem related to calling target_code in allocate_target_temps. Don't have more time today so need to stop here.

This only has an effect in the following testcase, which is not commited yet (so tests passes but things are not OK):

"""
>>> f()
a
"""

class A:
    attr = 0
    def __add__(self, other):
        return self
    def __repr__(self):
        return self.attr

def f():
    a = A()
    (a + 32).attr = "a"
    return a

comment:3 Changed 6 years ago by dagss

Buffer access towards complex types (treating them as a struct of two
floats) are now up in -dagss.

In order to get there I had to do a couple of dangerous things regarding
result_code though. These patches are really small but someone with more
knowledge may have more or less confidence in them than I:

http://hg.cython.org/cython-dagss/rev/b2bcc32021fd
http://hg.cython.org/cython-dagss/rev/a8a9e4c7d5bd

Note that this should fix the CloneNode? issue I talked about. (And all
tests pass btw; yet this is the kind of thing that would cause
horrendous failures in obscure cases which are likely not tested for...)

Also in -dagss I've introduces NewTempExprNode?, which is a transitionary
class where the temp allocation is moved to code generation. Ideally all
ExprNodes? should descend from this one instead (and then it should be
folded back into ExprNode? to complete the migration).

Some classes fail hard when they are changed to the new scheme. It works
for BinOpNode? which now uses it. This should allow partial migration.
(One must make sure a class can work with the new temp allocation scheme
before constructing them in a transform after analysis).

comment:4 Changed 6 years ago by dagss

There's been a long discussion about this now. See #124, some other tickets, and also http://search.gmane.org/search.php?group=gmane.comp.python.cython.devel&query=temp+allocation+flow

comment:5 Changed 6 years ago by dagss

#149 must be fixed before more nodes are converted to make the matter worse.

comment:6 Changed 3 months ago by scoder

  • Resolution set to fixed
  • Status changed from new to closed

This is long fixed, marking as done.

Note: See TracTickets for help on using tickets.