Request for reviews (M): 7069452: Cleanup NodeFlags

[Tom Rodriguez]

That looks good.

It looks like we could expand the number of bits dedicated to the class_id if we wanted to.  Would that allow us to have more complete coverage in the is_Node tests we provide?

tom

On Jul 26, 2011, at 7:47 PM, Vladimir Kozlov wrote:

> I updated webrev.
> 
> http://cr.openjdk.java.net/~kvn/7069452/webrev
> 
> is_Goto() is replaced with class_id check is_MachGoto(), new MachGotoNode is added.
> 
> Flag_is_block_start flag check replaced with is_Start() check.
> 
> is_safepoint_node() check is replaced with !is_MachCallLeaf() check. Corresponding code in adlc is removed since it is not needed.
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Jul 26, 2011, at 5:05 PM, Vladimir Kozlov wrote:
>>> Tom Rodriguez wrote:
>>>> So is_Goto returns false for GotoNode?  That doesn't seem right.
>>> It is used only for Mach nodes in block.cpp.
>> Then maybe it should be is_MachGoto or we should introduce a MachGotoNode.
>> tom
>>>> +bool Node::is_Goto() const { return is_Branch() && is_Mach() && (as_Mach()->ideal_Opcode() == Op_Goto); }
>>>> why does this have an is_Branch test?  Isn't testing the ideal_Opcode sufficient for the mach case?
>>> is_Branch() is bits test and ideal_Opcode() is virtual call. So it is a little speedup. I can set is_Branch flag in ideal GotoNode and change is_Goto() to:
>>> 
>>> bool Node::is_Goto() const {
>>> if (!is_Branch()) return false;
>>> if (is_Mach()) return (as_Mach()->ideal_Opcode() == Op_Goto);
>>> else           return (Opcode() == Op_Goto);
>>> }
>>> 
>>> 
>>> I will look on Flag_is_block_start and Flag_is_safepoint_node.
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> Flag_is_block_start seems to be minimally useful since it's only used to mark StartNode, at least as far as I can see.
>>>> Flag_is_safepoint_node could probably be eliminated without too much work too.  It's set for MachSafePointNode and cleared for MachCallLeaf.  The adlc tries to clear it for FastUnlock but those inherit from MachNode, not MachSafePointNode so it seems slightly useless.
>>>> tom
>>>> On Jul 22, 2011, at 11:06 AM, Vladimir Kozlov wrote:
>>>>> http://cr.openjdk.java.net/~kvn/7069452/webrev
>>>>> 
>>>>> Fixed 7069452: Cleanup NodeFlags
>>>>> 
>>>>> We are almost out of bits (16) for Node::NodeFlags. I removed flags which duplicate information in Node::NodeClasses.
>>>>> is_Call() uses class_id check and is now valid only for ideal CallNode.
>>>>> is_Goto() checks ideal_opcode().
>>>>> is_Vector() is replaced with check for Vector,VectorLoad,VectorStore classes.
>>>>> MachProjNode was added to class_id check to avoid calling Opcode() in many places. MulNode was removed from class_id since it was used only in one place.
>>>>> 
>>>>> I removed all logic associated with is_pc_relative flag. It was only checked in one place during long to short branch replacement. I replaced it with check in adlc parser - short branches should be defined only for a branch to a label, which means branch with PC relative offset. To relay on developer to set ins_pc_relative() was mistake since it could be used in wrong places. For example, it was specified for table jumps, calls and FastLock/FastUnlock.