[Vm-dev] something fishy with genPrimitiveStringReplace

Mon Nov 27 07:20:53 UTC 2017

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 <
nicolas.cellier.aka.nice at gmail.com> wrote:

>
>
>
> 2017-11-26 23:30 GMT+01:00 Nicolas Cellier <nicolas.cellier.aka.nice@
> gmail.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/307544570
>>>>>>>>
>>>>>>>> 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/ab3695d9/attachment.html>