something fishy with genPrimitiveStringReplace
Locked
something fishy with genPrimitiveStringReplace
 
|
the testTextReplacement3 now fails randomly. It sometimes omit to signal an Error... |
Re: something fishy with genPrimitiveStringReplace
|
|
Ok I have a look. This is on byte objects... On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
Bug seems to be with underflow access on replacement array: (1 to: 10) collect: [:i| ['123456789' replaceFrom: 1 to: 4 with: 'abcdefgh' startingAt: 0] on: Error do: ['error']] #('error' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789' ' abc56789'). Same thing on arrays since this is common code... Generated code looks ok... I need to look again I may have inverted some branches. On Sun, Nov 26, 2017 at 9:37 PM, Clément Bera <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
I think those lines are the problem: "0 >= start, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLess: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLess: 0. If this is equal if should jump out of bounds. On Sun, Nov 26, 2017 at 9:53 PM, Clément Bera <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
This is correct: (lessOrEqual instead of less):
"0 >= start, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: startReg. jumpOutOfBounds1 := cogit JumpLessOrEqual: 0. "0 >= replStart, fail" cogit CmpCq: (objectMemory integerObjectOf: 0) R: repStartReg. jumpOutOfBounds2 := cogit JumpLessOrEqual: 0. I can't commit right now (my image has many changes I need to commit and I cannot branch easily on monticello...) Will do tomorrow. Thanks for reporting. On Sun, Nov 26, 2017 at 10:58 PM, Clément Bera <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
Ok fixed in 2282 ! Thanks for reporting On Sun, Nov 26, 2017 at 11:04 PM, Clément Bera <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
Ah thanks! But I think that there is another bug in replacement size test...a := String new: n. b := String new: n-1. ([a replaceFrom: 1 to: n with: b startingAt: 1] ifError: [nil]) isNil. ] -> #(2 4 6 8 10 12 14 16 18 20) I'd expect they all fail, thus an empty #() 2017-11-26 23:09 GMT+01:00 Clément Bera <[hidden email]>:
|
Re: something fishy with genPrimitiveStringReplace
|
|
2017-11-26 23:30 GMT+01:00 Nicolas Cellier <[hidden email]>:
OK, I corrected it, the replArray size was not correct (VMMaker.oscog-nice.2283) The variable holding replArray format was already overwritten. So I choosed to store replArray format into repStartReg and that works OK.
|
Re: something fishy with genPrimitiveStringReplace
|
|
Right thanks. I need to check if those tests are in Pharo somewhere and add them. This primitive was tricky to implement. Not enough registers, it makes things difficult. With a couple more registers, things would be easier. We could try to change it so that the ARM and x64 version can use Extra0Reg/Extra1Reg and avoid reloading registers all the time. I'm thinking maybe something like: extraTempReg := backEnd hasExtra0Reg ifTrue: [Extra0Reg] ifFalse: [startReg]. And then later... backEnd hasExtra0Reg ifFalse: [cogit genStackArgAt: 3 into: startReg]. would not be that hard to implement and would speed things on x64/ARM (2-3 less instructions which are L1 cache memory reads). However the main point of this intrinsic is to avoid the switch to the C stack for small copies, speeding those drastically, and there is no spill/reload in the copying loop so no slow down on long copies, so maybe it's better to keep a single version which is already difficult to get right... On Mon, Nov 27, 2017 at 12:12 AM, Nicolas Cellier <[hidden email]> wrote:
Clément Béra Pharo consortium engineer Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq |
Re: something fishy with genPrimitiveStringReplace
|
|
2017-11-27 8:20 GMT+01:00 Clément Bera <[hidden email]>:
Of course, this is assembler... I clearly failed to review the generator code; such
review requires good knowledge of generic instruction set, which I haven't + mental map
of register usage... Despite your worthy efforts for using semantic names, this register scarcity indeed make such code kind of write only...Tests are there for polishing the corners ;)
It would then make generator code worse indeed. IMO, there are lower fruits hanging now.
|
«
Return to Squeak VM
|
1 view|%1 views
| Free forum by Nabble | Edit this page |