Tradeoffs around Node::Opcode()?

[Peter B. Kessler]

Thanks for the answers.

The _class_id field is extremely dense.  It's sort of like a tree path (left or right child, and then a sibling count), but different.  I like the idea of putting all the details about a class in the constructor for that class.

I didn't understand why you say that a subclass would overwrite an _opcode field.  If each AddI constructor initializes my proposed (const?) _opcode field in Node to Op_AddI, and if AddF initializes the _opcode field to Op_AddF, then who would overwrite it?  Obviously node instances can't change their opcodes, or we wouldn't be able to use a virtual function for Opcode() now, unless we change the vtable for instances.

Then there could be a map from Opcodes to what's now in _class_id and _flags.  E.g., there would be a _class_id table someplace whose Op_AddI entry would contain Class_Add.  As would the Op_AddF entry.  But the Op_AddP index would would contain Class_AddP.  Do the _flags fields change over the life of a Node instance?  Or could those also be in a map indexed by Opcode?  (Having separate maps from Opcode to other properties of the Node means that information about the Node subclass isn't visible in text of the subclass constructor.  Sigh.)

I hear from Chris Vick that making the edges between the nodes bi-directional, which increased the size of the Node instances, didn't have any noticeable effect on the space used collectively by the nodes during a compilation, and didn't have any effect on the speed of the compiler, except maybe to speed it up because traversals that used to be hard with one-way edges became easier with bi-directional edges.

			... peter

Vladimir Kozlov wrote:
> Yes, it is history :)
> 
> The _opcode field will not work since subclasses will overwrite it.
> 
> The _class_id field is not direct map to Opcode, it is bitmap:
> n->is_SafePoint() will be true for all ideal Call and SafePoint nodes,
> but n->Opcode() == Op_SafePoint only true for SafePointNode.
> So we need both.
> 
> And yes, I want to fit in a jushort :-)
> When I did implementation of _class_id I was also concern about
> increasing Node size significantly so I decided to add only 4 bytes:
> 2 for class_id and 2 for flags.
> 
> I also did profiling of Opcode() usage. So only hottest Opcode()
> calls were replaced with class_id code. Adding other nodes did not
> improve performance.
> 
> Vladimir
> 
> Peter B. Kessler wrote:
>> Cool!  Thanks.  So much for engineering: we already have *both* the 
>> virtual dispatch *and* the data slot per instance.
>>
>> Oh, but those are only the class id's (e.g., Add), rather than the 
>> opcode (e.g., AddI).  It's confusing that they are both referred to as 
>> Node "classes", e.g., in classes.hpp, opcodes.cpp, etc.
>>
>> So I guess that begs the question: why *isn't* there an _opcode field, 
>> initialized in the constructors, like (or instead of) _class_id?  Why 
>> aren't the leaf classes (AddI, etc.) in the _class_id maps?  (Other 
>> than that the _class_id maps want to fit in a jushort. :-)  Couldn't 
>> one map (e.g., with a table) from an opcode to a _class_id, if one 
>> wanted to spend the time as opposed to the space?  Would an inline 
>> method to access a _class_id table be faster than a virtual dispatch 
>> to Opcode()?
>>
>> A possible answer as to why it is the way it is: "History".
>>
>> I'm still trying to understand the tradeoffs between space and time.
>>
>>             ... peter
>>
>> Vladimir Kozlov wrote:
>>> Peter,
>>>
>>> There is already such functionality exactly for such purpose.
>>> The field is jushort _class_id; which use enum NodeClasses
>>> and initializer in the constructors init_class_id().
>>>
>>> Vladimir
>>>
>>> Peter B. Kessler wrote:
>>>> A lot of things in class Node are organized around having a dense 
>>>> integer range to identify the Node.  That's Node::Opcode().
>>>>
>>>> I'm wondering about the engineering around that method.  
>>>> Node::Opcode() is a virtual call, to get from a Node instance to a 
>>>> piece of data on the Node subclass of the instance.  An alternative 
>>>> would be to add a data field in Node to hold the opcode, and replace 
>>>> the virtual Node::Opcode() calls with an inline method to return the 
>>>> contents of that slot.  That trades the virtual dispatch cost for 
>>>> each call with a space cost in each instance.
>>>>
>>>> Does anyone have any feeling about whether that's a good tradeoff?  
>>>> How often is Node::Opcode() called?  What effect would it have to 
>>>> make it run faster?  How important is it to minimize the size of 
>>>> Node instances?  (There are other places where effort is expended to 
>>>> keep Node's small.)
>>>>
>>>> Thanks for any opinions on this.
>>>>
>>>>             ... peter
>>