Request for reviews (M): 7069452: Cleanup NodeFlags (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Jul 27 17:20:53 PDT 2011

Thank you, Tom

Yes, we have few free bits in _flags now, but it does not mean we will have them tomorrow :) I will take one for next cbcond changes to mark avoid_back_to_back instructions.

I don't think we need to cover all classes in class_id, we can use Opcode(). At the beginning I did profiling of Opcode() usage and replaced only hot ones with class_id check but now it is expanded for other usages. We still have space in class_id so I would not expand it now.

Thanks, Vladimir

Tom Rodriguez wrote:

That looks good.

It looks like we could expand the number of bits dedicated to the classid if we wanted to. Would that allow us to have more complete coverage in the isNode 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 isGoto() is replaced with classid check isMachGoto(), new MachGotoNode is added. Flagisblockstart flag check replaced with isStart() check. issafepointnode() check is replaced with !isMachCallLeaf() 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 isGoto returns false for GotoNode? That doesn't seem right. It is used only for Mach nodes in block.cpp. Then maybe it should be isMachGoto or we should introduce a MachGotoNode. tom +bool Node::isGoto() const { return isBranch() && isMach() && (asMach()->idealOpcode() == OpGoto); } why does this have an isBranch test? Isn't testing the idealOpcode sufficient for the mach case? isBranch() is bits test and idealOpcode() is virtual call. So it is a little speedup. I can set isBranch flag in ideal GotoNode and change isGoto() to:

bool Node::isGoto() const { if (!isBranch()) return false; if (isMach()) return (asMach()->idealOpcode() == OpGoto); else return (Opcode() == OpGoto); }

I will look on Flagisblockstart and Flagissafepointnode. Thanks, Vladimir Flagisblockstart seems to be minimally useful since it's only used to mark StartNode, at least as far as I can see. Flagissafepointnode 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. isCall() uses classid check and is now valid only for ideal CallNode. isGoto() checks idealopcode(). isVector() is replaced with check for Vector,VectorLoad,VectorStore classes. MachProjNode was added to classid check to avoid calling Opcode() in many places. MulNode was removed from classid since it was used only in one place. I removed all logic associated with ispcrelative 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 inspcrelative() was mistake since it could be used in wrong places. For example, it was specified for table jumps, calls and FastLock/FastUnlock.

More information about the hotspot-compiler-dev mailing list