[Vm-dev] something fishy with genPrimitiveStringReplace

Mon Nov 27 09:14:54 UTC 2017

2017-11-27 8:20 GMT+01:00 Clément Bera <bera.clement at gmail.com>:

>
> 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.
>
> 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...

Thanks for writing the tough part anyway!
Tests are there for polishing the corners ;)

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...
>
>
It would then make generator code worse indeed.
IMO, there are lower fruits hanging now.

On Mon, Nov 27, 2017 at 12:12 AM, Nicolas Cellier <nicolas.cellier.aka.nice@
> gmail.com> wrote:
>
>>
>>
>>
>> 2017-11-26 23:30 GMT+01:00 Nicolas Cellier <nicolas.cellier.aka.nice at gmai
>> l.com>:
>>
>>> Ah thanks!
>>>
>>> But I think that there is another bug in replacement size test...
>>>
>>> (1 to: 20) reject: [:n |
>>>   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 #()
>>>
>>
>> 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.
>>
>>
>>> 2017-11-26 23:09 GMT+01:00 Clément Bera <bera.clement at gmail.com>:
>>>
>>>>
>>>> Ok fixed in 2282 ! Thanks for reporting
>>>>
>>>> On Sun, Nov 26, 2017 at 11:04 PM, Clément Bera <bera.clement at gmail.com>
>>>> wrote:
>>>>
>>>>> 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 <bera.clement at gmail.com
>>>>> > wrote:
>>>>>
>>>>>> 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 <bera.clement at gmail.com
>>>>>> > wrote:
>>>>>>
>>>>>>> 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 <
>>>>>>> bera.clement at gmail.com> wrote:
>>>>>>>
>>>>>>>> Ok I have a look.
>>>>>>>>
>>>>>>>> This is on byte objects...
>>>>>>>>
>>>>>>>> On Sun, Nov 26, 2017 at 9:07 PM, Nicolas Cellier <
>>>>>>>> nicolas.cellier.aka.nice at gmail.com> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Like reported on travis
>>>>>>>>> https://travis-ci.org/OpenSmalltalk/opensmalltalk-vm/jobs/30
>>>>>>>>> 7544570
>>>>>>>>>
>>>>>>>>> the testTextReplacement3 now fails randomly.
>>>>>>>>> It sometimes omit to signal an Error...
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Clément Béra
>>>>>>>> Pharo consortium engineer
>>>>>>>> https://clementbera.wordpress.com/
>>>>>>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Clément Béra
>>>>>>> Pharo consortium engineer
>>>>>>> https://clementbera.wordpress.com/
>>>>>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Clément Béra
>>>>>> Pharo consortium engineer
>>>>>> https://clementbera.wordpress.com/
>>>>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Clément Béra
>>>>> Pharo consortium engineer
>>>>> https://clementbera.wordpress.com/
>>>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Clément Béra
>>>> Pharo consortium engineer
>>>> https://clementbera.wordpress.com/
>>>> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>>>>
>>>>
>>>
>>
>>
>
>
> --
> Clément Béra
> Pharo consortium engineer
> https://clementbera.wordpress.com/
> Bâtiment B 40, avenue Halley 59650 Villeneuve d'Ascq
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20171127/900751ad/attachment-0001.html>