Thanks Henry. Your approach seems much more rational. For example,e, branches are to the start of a byte code sequence, and return addresses are to the start of the byte code following the send. There is no case I can think of when the last byte of a byte code is an important ordinal, not even in cannotReturn: or illegalBytecode:.
On Mon, Oct 12, 2015 at 7:37 AM, Henrik Johansen < henrik.s.johan...@veloxit.no> wrote: > > On 12 Oct 2015, at 3:43 , Henrik Johansen <henrik.s.johan...@veloxit.no> > wrote: > > > On 10 Oct 2015, at 10:05 , Nicolai Hess <nicolaih...@web.de> wrote: > > > > 2015-10-08 23:45 GMT+02:00 Eliot Miranda <eliot.mira...@gmail.com>: > >> Hi Nicolai, >> >> On Thu, Oct 8, 2015 at 2:27 PM, Nicolai Hess <nicolaih...@web.de> wrote: >> >>> Hello Eliot, and thanks for your time. >>> >>> >>> In our (pharo) implementation for sourceNodeForPC, we start at the >>> AST-Node of >>> the compiled method, get the IR (IntermediateRepresentation) and scan >>> this sequence of IRNodes >>> until we finally find that one, that maps to the bytecode of the closure >>> creation code (this should be a IRPushClosureCopy). >>> The problem is now, we don't find (always) the right IRNode, and it >>> depens on the number of local temps, if this is > 3 we don't hit the right >>> one. >>> >> >> That's very strange. Can you construct a synthetic example of two simple >> blocks, one with three temps and and one with four temps and show me the >> difference in the byte code? >> > > > [ :x | | a b c | a:=b:=c:=x ] "calling #sourceNode gives a > RBBlockNode - ok" > Bytecode > 13 <8F 01 00 0B> closureNumCopied: 0 numArgs: 1 bytes 17 to 27 > 17 <73> pushConstant: nil > 18 <73> pushConstant: nil > 19 <73> pushConstant: nil > 20 <10> pushTemp: 0 > 21 <81 43> storeIntoTemp: 3 > 23 <81 42> storeIntoTemp: 2 > 25 <81 41> storeIntoTemp: 1 > 27 <7D> blockReturn > 28 <7C> returnTop > > > [ :x | | a b c d | a:=b:=c:=d:=x ] "calling #sourceNode gives a > RBMethodNode - not ok" > Bytecode > 13 <8F 01 00 0E> closureNumCopied: 0 numArgs: 1 bytes 17 to 30 > 17 <73> pushConstant: nil > 18 <73> pushConstant: nil > 19 <73> pushConstant: nil > 20 <73> pushConstant: nil > 21 <10> pushTemp: 0 > 22 <81 44> storeIntoTemp: 4 > 24 <81 43> storeIntoTemp: 3 > 26 <81 42> storeIntoTemp: 2 > 28 <81 41> storeIntoTemp: 1 > 30 <7D> blockReturn > 31 <7C> returnTop > > As I wrote above, in Pharo, to get the sourceNode we try to find the push > closure bytecode from "startpc - 1" by traversing > the IR and try to get the IRNode with matching "bytecodeOffset". The > bytecodeOffsets for the above blocks: > > [ :x | | a b c | a:=b:=c:=x ] > "bytecodeOffset -> IR" > 19->pushClosureCopyCopiedValues: #() args: #(#x) > 28->returnTop > 20->pushTemp: #x > 22->storeTemp: #c > 24->storeTemp: #b > 26->storeTemp: #a > 27->blockReturnTop > > [ :x | | a b c d | a:=b:=c:=d:=x ] > "bytecodeOffset -> IR" > 20->pushClosureCopyCopiedValues: #() args: #(#x) > 31->returnTop > 21->pushTemp: #x > 23->storeTemp: #d > 25->storeTemp: #c > 27->storeTemp: #b > 29->storeTemp: #a > 30->blockReturnTop > > Both blocks don't have a bytecodeOffset matching the bytecode of the > pushclosure (13), but we start the search from "startpc - 1" (= 16) > and do a loop from 0 to -3 (because bytecodes can have a length of 1,2 or > 4) > For the first block, this works > bytecodeOffset 19 = 16 - (-3) > For the second block this does not work, somehow the bytecodeoffset value > depends on the number of local temps. And for the > second block this is one greater. > > > > >> And my interpretation was, the IRPushClosureCopy node has the wrong >>> "bytecode offset" (whatever this is). The bytecodeOffset value (somehow) >>> depends on the number of local temps, but maybe this is right (for >>> whatever this is used) and we are just using it wrong for in this situation. >>> >> >> So you need to ask Marcus how to interpret bytecodeOffset. The thing is, >> it should be simple. One has: >> >> BlockCreationBytecode >> any number of pushConstant: nil's (0 to N) >> first byte code in block >> last bytecode in block (always a blockReturnTop) >> >> and a block's startup is always that of the byte immediately following >> the BlockCreationBytecode. So if the byte code offset is somehow the span >> of the "any number of pushConstant: nil's (0 to N)" then there's a bug in >> the IR somewhere when the number of temps is > 3. But if that's not what >> it means then, well, I don't know what the problem is. >> >> > > For some reason, bytecodeOffset is the *last* byte associated with IR. > This makes little sense to me, the main use seems to be to get correct > offsets for different instructions in an IR sequence. > Since block IR includes temp initialization bytes, the offset is 5. > > Changing the code to record *starting* offset of an IRNode in IRSequence, > by: > mapBytesTo: instr > "Associate the current byte offset with instr. We fix this later to have > the correct offset, > see #bytecodes" > instrMap add: instr -> (bytes size + 1) > > and mapping bytes *before* visiting node in the IRTranslatorV2: > visitInstruction: instr > gen mapBytesTo: instr. > self visitNode: instr. > > and the same in visitPushClosureCopy: closure > closure copiedValues do: [:name | > gen mapBytesTo: closure. > gen pushTemp: (self currentScope indexForVarNamed: name). > ]. > --snip-- > > > Hupps, the pushing of copiedValues are not part of the closure bytecode, > so that needs to be > closure copiedValues do: [:name | > gen pushTemp: (self currentScope indexForVarNamed: name). > ]. > gen mapBytesTo: closure. > > Thanks testBlockAndContextSourceNode! > > After a recompile of the test methods in case of cached IR (and offsets) > (RPackageOrganizer default packageNamed: 'OpalCompiler-Tests') classes > do:#recompile > All (existing) tests are green with the attached change set (where > haltOnce's were removed too), at least in the almost-release version of > Pharo4 I tested in. > > Cheers, > Henry > > > > > > -- _,,,^..^,,,_ best, Eliot
