Suggested change for SpurMemoryManager>>#classAtIndex:

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Suggested change for SpurMemoryManager>>#classAtIndex:

Hernan Wilkinson-3
 
Hi,
 I got compile errors when setting <inline: true> in a method I created.
 The errors where a label redefinition such as:
../../spurstack64src/vm/gcc3x-interp.c:4790:2: error: redefinition of label 'l35'
        l35:    /* end classAtIndex: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4634:2: note: previous definition is here
        l35:    /* end classAtIndex: */;
        ^

 After some research I concluded that the problem was at SpurMemoryManager>>#classAtIndex: due to how it returns:
...
classTablePage = nilObj ifTrue:
[^nil].
^self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage

 I think the source code generator does not support correctly returning at different points when inlining the same method more than once. 
 So I changed that implementation to:
...
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Doing so the compile errors were gone and the VM keeps working correctly.
 Is this fix correct or does it have some performance hit I do not know?
 Should I inform about this change with a PR? (Sorry about this question but I'm new to this vm stuff :-) )

 Here is the complete code of the method:
classAtIndex: classIndex
<api>
<inline: true>
| classTablePage |
self assert: (classIndex >= 0 and: [classIndex <= self tagMask or: [classIndex >= self arrayClassIndexPun and: [classIndex <= self classIndexMask]]]).
classTablePage := self fetchPointer: classIndex >> self classTableMajorIndexShift
ofObject: hiddenRootsObj.
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Cheers!
 Hernan.
--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina
Reply | Threaded
Open this post in threaded view
|

Re: Suggested change for SpurMemoryManager>>#classAtIndex:

Hernan Wilkinson-3
 
well, I have the same problem but with the message #lengthOf:, but now the behavior is weird.
If I have the following code, everything works fine:
typesSize := (objectMemory lengthOf: types) - 1.

If I remove the - 1, like this:
typesSize := objectMemory lengthOf: types.

I get redefinition of labels errors:
../../spurstack64src/vm/gcc3x-interp.c:4879:2: error: redefinition of label 'l46'
        l46:    /* end lengthOf:format: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4693:2: note: previous definition is here
        l46:    /* end lengthOf:format: */;
        ^

The thing is that in the first case #lengthOf: is not inlined but in the second it is and it looks like that the label is not regenerated every time it gets inlined.
 Is there a well know workaround for this? I mean, I could do:
typesSize := (objectMemory lengthOf: types) - 0.
 
 and it will compile but that is not the point.
 Also, why in the first case the code generator decides not to inline #lengthOf: but it does in the second case?

Thanks!
Hernan.



On Tue, Oct 9, 2018 at 8:40 PM Hernan Wilkinson <[hidden email]> wrote:
Hi,
 I got compile errors when setting <inline: true> in a method I created.
 The errors where a label redefinition such as:
../../spurstack64src/vm/gcc3x-interp.c:4790:2: error: redefinition of label 'l35'
        l35:    /* end classAtIndex: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4634:2: note: previous definition is here
        l35:    /* end classAtIndex: */;
        ^

 After some research I concluded that the problem was at SpurMemoryManager>>#classAtIndex: due to how it returns:
...
classTablePage = nilObj ifTrue:
[^nil].
^self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage

 I think the source code generator does not support correctly returning at different points when inlining the same method more than once. 
 So I changed that implementation to:
...
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Doing so the compile errors were gone and the VM keeps working correctly.
 Is this fix correct or does it have some performance hit I do not know?
 Should I inform about this change with a PR? (Sorry about this question but I'm new to this vm stuff :-) )

 Here is the complete code of the method:
classAtIndex: classIndex
<api>
<inline: true>
| classTablePage |
self assert: (classIndex >= 0 and: [classIndex <= self tagMask or: [classIndex >= self arrayClassIndexPun and: [classIndex <= self classIndexMask]]]).
classTablePage := self fetchPointer: classIndex >> self classTableMajorIndexShift
ofObject: hiddenRootsObj.
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Cheers!
 Hernan.
--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina
Reply | Threaded
Open this post in threaded view
|

Re: Suggested change for SpurMemoryManager>>#classAtIndex:

Clément Béra
In reply to this post by Hernan Wilkinson-3
 
Hi,

answers inlined

On Wed, Oct 10, 2018 at 1:40 AM Hernan Wilkinson <[hidden email]> wrote:
 
Hi,
 I got compile errors when setting <inline: true> in a method I created.
 The errors where a label redefinition such as:
../../spurstack64src/vm/gcc3x-interp.c:4790:2: error: redefinition of label 'l35'
        l35:    /* end classAtIndex: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4634:2: note: previous definition is here
        l35:    /* end classAtIndex: */;
        ^

 After some research I concluded that the problem was at SpurMemoryManager>>#classAtIndex: due to how it returns:
...
classTablePage = nilObj ifTrue:
[^nil].
^self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage

 I think the source code generator does not support correctly returning at different points when inlining the same method more than once. 
 So I changed that implementation to:
...
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Doing so the compile errors were gone and the VM keeps working correctly.
 Is this fix correct or does it have some performance hit I do not know? 
 Should I inform about this change with a PR? (Sorry about this question but I'm new to this vm stuff :-) )

 
It looks good to me. This can be integrated.
This is versioned in Monticello. So I guess you can commit if you have the right and if there's a problem we will ignore the commit and tell on the mailing list why.
 
 Here is the complete code of the method:
classAtIndex: classIndex
<api>
<inline: true>
| classTablePage |
self assert: (classIndex >= 0 and: [classIndex <= self tagMask or: [classIndex >= self arrayClassIndexPun and: [classIndex <= self classIndexMask]]]).
classTablePage := self fetchPointer: classIndex >> self classTableMajorIndexShift
ofObject: hiddenRootsObj.
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Cheers!
 Hernan.
--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
Reply | Threaded
Open this post in threaded view
|

Re: Suggested change for SpurMemoryManager>>#classAtIndex:

Clément Béra
In reply to this post by Hernan Wilkinson-3
 
Hi again,

answers inlined again.

On Wed, Oct 10, 2018 at 11:45 AM Hernan Wilkinson <[hidden email]> wrote:
 
well, I have the same problem but with the message #lengthOf:, but now the behavior is weird.
If I have the following code, everything works fine:
typesSize := (objectMemory lengthOf: types) - 1.

If I remove the - 1, like this:
typesSize := objectMemory lengthOf: types.

I get redefinition of labels errors:
../../spurstack64src/vm/gcc3x-interp.c:4879:2: error: redefinition of label 'l46'
        l46:    /* end lengthOf:format: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4693:2: note: previous definition is here
        l46:    /* end lengthOf:format: */;
        ^

The thing is that in the first case #lengthOf: is not inlined but in the second it is and it looks like that the label is not regenerated every time it gets inlined.
 Is there a well know workaround for this? I mean, I could do:
typesSize := (objectMemory lengthOf: types) - 0.

Looks like a bug in the slang inliner. The Slang to C compiler is able to compile the VM, and that's it, so if you do anything that's not already used, it basically has undefined behavior (may or may not work). That's probably known and it would be better if Eliot answers about that.
 
 and it will compile but that is not the point.
 Also, why in the first case the code generator decides not to inline #lengthOf: but it does in the second case?

There's two inlining strategies. On the JIT side it's explicit, so inline: true will inline, anything else is not inlined. The key aspect there is to inline non C compatible code (closures, alloca interactions). On the interpreter side it's heuristic based. It's quite tricky because you want to control carefully what is inlined in the main interpreter loop & on performance critical functions, while keeping enough information for profiling. You can fine-tune the heuristics with inline: false/true. You can look into the details in the CCodeGenerator class. Then there are the work-arounds, for instance since linking time optimizations are not enabled, quick calls across files are typically defined as macros to be inlined. Again, it would be better if Eliot answers about that, he has better knowledge on this.


Thanks!
Hernan.



On Tue, Oct 9, 2018 at 8:40 PM Hernan Wilkinson <[hidden email]> wrote:
Hi,
 I got compile errors when setting <inline: true> in a method I created.
 The errors where a label redefinition such as:
../../spurstack64src/vm/gcc3x-interp.c:4790:2: error: redefinition of label 'l35'
        l35:    /* end classAtIndex: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4634:2: note: previous definition is here
        l35:    /* end classAtIndex: */;
        ^

 After some research I concluded that the problem was at SpurMemoryManager>>#classAtIndex: due to how it returns:
...
classTablePage = nilObj ifTrue:
[^nil].
^self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage

 I think the source code generator does not support correctly returning at different points when inlining the same method more than once. 
 So I changed that implementation to:
...
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Doing so the compile errors were gone and the VM keeps working correctly.
 Is this fix correct or does it have some performance hit I do not know?
 Should I inform about this change with a PR? (Sorry about this question but I'm new to this vm stuff :-) )

 Here is the complete code of the method:
classAtIndex: classIndex
<api>
<inline: true>
| classTablePage |
self assert: (classIndex >= 0 and: [classIndex <= self tagMask or: [classIndex >= self arrayClassIndexPun and: [classIndex <= self classIndexMask]]]).
classTablePage := self fetchPointer: classIndex >> self classTableMajorIndexShift
ofObject: hiddenRootsObj.
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Cheers!
 Hernan.
--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
Reply | Threaded
Open this post in threaded view
|

Re: Suggested change for SpurMemoryManager>>#classAtIndex:

David T. Lewis
In reply to this post by Clément Béra
 
On Thu, Oct 11, 2018 at 10:43:37AM +0200, Cl??ment B??ra wrote:

>  
> On Wed, Oct 10, 2018 at 1:40 AM Hernan Wilkinson <
> [hidden email]> wrote:
>
> >
>  Should I inform about this change with a PR? (Sorry about this question
> > but I'm new to this vm stuff :-) )
> >
> >
> It looks good to me. This can be integrated.
> This is versioned in Monticello. So I guess you can commit if you have the
> right and if there's a problem we will ignore the commit and tell on the
> mailing list why.
>

For submitting changes like this, you may also want to take advantage
of the recently created VMMaker inbox repository:

  http://source.squeak.org/VMMakerInbox

This is set up so that anyone can commit to the inbox. The changes
can then be discussed on the list, and can be easily moved to (or
merged with) the main VMMaker repository.

Signing up for an account on source.squeak.org is encouraged but
not required. You should be able to commit anonymously if you prefer.

HTH,
Dave

Reply | Threaded
Open this post in threaded view
|

Re: Suggested change for SpurMemoryManager>>#classAtIndex:

Eliot Miranda-2
In reply to this post by Hernan Wilkinson-3
 
Hi Hernan,
On Wed, Oct 10, 2018 at 2:45 AM Hernan Wilkinson <[hidden email]> wrote:
 
well, I have the same problem but with the message #lengthOf:, but now the behavior is weird.
If I have the following code, everything works fine:
typesSize := (objectMemory lengthOf: types) - 1.

If I remove the - 1, like this:
typesSize := objectMemory lengthOf: types.

I get redefinition of labels errors:
../../spurstack64src/vm/gcc3x-interp.c:4879:2: error: redefinition of label 'l46'
        l46:    /* end lengthOf:format: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4693:2: note: previous definition is here
        l46:    /* end lengthOf:format: */;
        ^

The thing is that in the first case #lengthOf: is not inlined but in the second it is and it looks like that the label is not regenerated every time it gets inlined.
 Is there a well know workaround for this? I mean, I could do:
typesSize := (objectMemory lengthOf: types) - 0.
 
 and it will compile but that is not the point.

This does look like a bug.  Can you mail me the complete method and I'll try and have a look (or could you commit the package to VMMakerInbox and tell me the method?).  I'd like to understand if this is a fixable bug or a limitation.
 
 Also, why in the first case the code generator decides not to inline #lengthOf: but it does in the second case?

The inlines chooses to inline methods not marked as inlined based on (IIRC) two properties:

a) if the thing being inlined is only used once it is always inlined
b) if the thing is short it will be inlined

So it is a bit arbitrary (and in my experience extremely fragile).  Things marked as <inline: true> or <inline: #always> always get inlined.  With <inline: #always> the original is not generated.  <inline: false> and <inline: #never> also don't get inlined, but in addition <inline: #never> generates a pragma for the C compiler to prevent the C compiler from inlining (which is useful for profiling or debugging).
 
Thanks!
Hernan.



On Tue, Oct 9, 2018 at 8:40 PM Hernan Wilkinson <[hidden email]> wrote:
Hi,
 I got compile errors when setting <inline: true> in a method I created.
 The errors where a label redefinition such as:
../../spurstack64src/vm/gcc3x-interp.c:4790:2: error: redefinition of label 'l35'
        l35:    /* end classAtIndex: */;
        ^
../../spurstack64src/vm/gcc3x-interp.c:4634:2: note: previous definition is here
        l35:    /* end classAtIndex: */;
        ^

 After some research I concluded that the problem was at SpurMemoryManager>>#classAtIndex: due to how it returns:
...
classTablePage = nilObj ifTrue:
[^nil].
^self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage

 I think the source code generator does not support correctly returning at different points when inlining the same method more than once. 
 So I changed that implementation to:
...
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Doing so the compile errors were gone and the VM keeps working correctly.
 Is this fix correct or does it have some performance hit I do not know?
 Should I inform about this change with a PR? (Sorry about this question but I'm new to this vm stuff :-) )

 Here is the complete code of the method:
classAtIndex: classIndex
<api>
<inline: true>
| classTablePage |
self assert: (classIndex >= 0 and: [classIndex <= self tagMask or: [classIndex >= self arrayClassIndexPun and: [classIndex <= self classIndexMask]]]).
classTablePage := self fetchPointer: classIndex >> self classTableMajorIndexShift
ofObject: hiddenRootsObj.
^classTablePage = nilObj 
ifTrue: [nil]
ifFalse: [ self
fetchPointer: (classIndex bitAnd: self classTableMinorIndexMask)
ofObject: classTablePage ]

 Cheers!
 Hernan.
--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
Hernán Wilkinson
Agile Software Development, Teaching & Coaching
Phone: +54-011-4893-2057
Twitter: @HernanWilkinson
Address: Alem 896, Floor 6, Buenos Aires, Argentina


--
_,,,^..^,,,_
best, Eliot