0xdaryl
commented
2 years ago After discussion at today's OMR Architecture Meeting (#6451) the consensus was to go ahead and introduce a new IL opcode to handle conversions from long types to collected references (GC objects allocated on a heap). The IL opcode will be treated as a conversion and some work in common code will need to occur to ensure it is handled properly. Also, each backend will need to implement support (should be a relatively minor amount of work).
Capturing an OpenJ9 Slack channel discussion from here: https://openj9.slack.com/archives/C8312LCV9/p1648614748554659
Hi, does anyone know how I can determine whether the register for l2a node should be a collected reference register or not? 12 replies
Background is, I found a problem with the base register for load instruction of object field. It was not set as collected reference register and caused a GC problem in https://github.com/eclipse-openj9/openj9/issues/14663. For example, the base register (GPR_755) in this IL tree is not a collected reference register because we skip l2a node when populating memory reference.
https://github.com/eclipse/omr/blob/72289b92a2a3b678d630ca21d5a3e1a5562968c6/compiler/aarch64/codegen/OMRMemoryReference.cpp#L534-L547
If I change memory reference class not to skip l2a, then I got another GC crash. In this case, the register for l2a (GPR_0018) should not a collected reference register because it contains the entry address of a jitted method.
On all platforms, l2aEvaluator basically does the same thing. It changes the register to a collected reference register if containsCompressionSequence is set on the child node or compressedrefs shift is 0. (and isl2aForCompressedArrayletLeafLoad is not set to the l2a node). https://github.com/eclipse/omr/blob/72289b92a2a3b678d630ca21d5a3e1a5562968c6/compiler/aarch64/codegen/UnaryEvaluator.cpp#L526-L527 https://github.com/eclipse/omr/blob/72289b92a2a3b678d630ca21d5a3e1a5562968c6/compiler/p/codegen/UnaryEvaluator.cpp#L574-L575 https://github.com/eclipse/omr/blob/72289b92a2a3b678d630ca21d5a3e1a5562968c6/compiler/x/codegen/UnaryEvaluator.cpp#L278-L279 https://github.com/eclipse/omr/blob/72289b92a2a3b678d630ca21d5a3e1a5562968c6/compiler/z/codegen/UnaryEvaluator.cpp#L251-L252
@jdmpapin 1 month ago l2a needs to be a collected reference iff its child is the address of a GC-managed object At any point (AFAIK) before compressed refs lowering, this means that l2a cannot be a collected reference, since we have no way to prevent the integer value of the address from getting out of date before l2a, or even to prevent the object at that address from getting collected (in case there are no GC-visible references to it) But then compressed refs lowering comes along and generates collected l2a nodes as well... With the evaluators you linked, I'm a little surprised we don't see more problems. For example, as you point out, they all seem to treat l2a as collected whenever we're using 0-shift compressed refs (except in the arraylet-related case), but that's incorrect in cases like the one in the second set of pasted trees, where some JCL type known to the compiler has a long field containing the address of a J9Class or J9Method, etc., which we then use via l2a, resulting in a subtree like l2a (lloadi MagicType.vmSlot) Maybe we don't see this latter problem much because it's unlikely for such an l2a to be commoned across a GC point
@jdmpapin 1 month ago As for the original problem, it seems at first glance that it should always be OK to skip evaluating an l2a with refcount 1 that feeds into an indirect load. The distinction between a collected and uncollected register only matters at GC points, and the l2a won't be holding the register live beyond any GC points in later trees However, the issue isn't really commoning. It's whether there is a GC point between the instruction that sets the register and any later instruction that consumes the register. In the first set of pasted trees, typeOfBuffer is unresolved, so evaluation of n4145n has generated a sequence that first gets the base object reference into a register (GPR_0755) and then resolves the field (GC point) before using it
@jdmpapin 1 month ago My inclination is to say that we should create a reliable way to determine whether l2a is collected. A few example ideas for how to do that:
@jdmpapin 1 month ago @vijaysun-omr, @0xdaryl, @mstoodle any thoughts on this? ^ Possible OMR architecture meeting topic?
@jdmpapin 1 month ago Alternatively, it might be possible to ensure that we generate code in which the GC point is strictly before any children are evaluated. I feel like that's a more error-prone way to go about it though, and the machine code might end up worse after resolution (edited)
@vijaysun-omr 1 month ago I feel that the new opcode l2gcref is the best of the options that you listed
@vijaysun-omr 1 month ago But we can have an architecture meeting discussion on it
@mstoodle 1 month ago i agree that a new opcode seems to fit our usual criteria, and I like it better than the other options you’ve enumerated. It almost feels like we should have promoted collected address to a data type, but that would be a much larger undertaking at this point that may not be warranted. It should be a good architecture meeting topic, and I hope I can attend that one!
@0xdaryl 1 month ago Yes, we can discuss options at an upcoming arch meeting. I talked with Saitoh-san tonight and he will implement a tactical solution in the memory reference evaluator that should be enough until we have a more strategic solution in place. :+1:
@akira1saitoh 1 month ago @jdmpapin Thank you for detailed explanation! :+1: