Facilitate recording of constants observed in call target selection #7282
Conversation
When using std::map and std::set directly, the TR::typed_allocator must always be specified explicitly. Since the allocator parameter is last, the comparator must be given explicitly as well even if the default of std::less would be appropriate, which is often the case. As a result, typical uses of std::map and std::set take up multiple lines just to write down the type. Additionally, when specifying the allocator for std::map, the key (first) type in the std::pair type provided to TR::typed_allocator must be const-qualified. In the past it has been possible to forget const, which resulted in strange consequences. For example, here's a commit adding a forgotten const: 10dbf7e. This commit defines subtypes TR::map and TR::set that take care of wrapping the allocator in TR::typed_allocator. The default allocator (TR::Region&) should almost always be usable, in which case it can be omitted, and it might also be possible to omit the comparator. (Note that this default differs from that of TR::vector and TR::list, but TR::Region& might arguably be a better default for those as well.)
If inlining uses constant folding to help determine call targets, then it's important for the corresponding IL (if any is generated) to be consistent with what the inliner saw. This commit equips TR_CallTarget with a collection of observed constants that must be folded consistently in IL. This collection can be populated by an inliner and later consulted by an IL generator to ensure that the operations are folded as needed. This repeated folding is important whenever a value might be allowed to be folded despite the possibility of a later change. It also allows constants to be speculative by specifying the assumptions that are necessary in order for the folding to be correct, and by informing the IL generator of the locations where such assumptions have been made.
jdmpapin
requested review from
vijaysun-omr,
Leonardo2718,
0xdaryl and
mstoodle
as code owners
|
@0xdaryl and/or @vijaysun-omr, please review |
| genILSucceeded = tryToGenerateILForMethod(calleeSymbol, callerSymbol, calltarget); | ||
| comp()->setCurrentILGenCallTarget(NULL); |
There was a problem hiding this comment.
If there is an exception during IL gen due to an unsupported bytecode for example, I assume nothing would go wrong since we would exit the scope where the setting of "current IL gen call target" to would matter
There was a problem hiding this comment.
If an exception propagates beyond this point, then yes, the compilation will fail as a whole, so the current IL gen call target won't matter anymore
Some exceptions are caught within the call (more specifically, in ResolvedMethodSymbol::genIL()), and they only cause IL generation to fail. I believe something like an unsupported bytecode would be one of those. In that case, we'd just set genILSucceeded=false and then still clear the current call target
|
"...it's important for the corresponding IL (if any is generated) to be consistent with what the inliner saw....". This sounds like it might be important both from a functional correctness and performance standpoint. While it is relatively easy to imagine how recording of folding that was done during call target selection could lead to better performance if made available during IL gen, I think any functional reasons may be more subtle. Could you please elaborate on this ? i.e. are there functional reasons for ensuring consistency here ? |
|
jenkins build all |
To hit a functional problem due to inconsistency, I believe it would need to be the case that a value can change, but that we're nonetheless allowed to fold it:
We're currently not allowed to do such folding for references because the compiler's known object handling isn't able to deal with it. But in practice we have done it because it's hard to avoid. We've ended up with some workarounds in cases where it has caused a noticeable problem, and we're probably still unknowingly doing this in some - potentially many - cases. eclipse-openj9/openj9#16616 is intended to allow such folding properly in the future. That said, at a high level the functional reason is that constant folding could influence the selection of which call targets to inline. This happens most obviously due to refinement of recognized methods. Let's consider a call to I don't think the importance is limited to specially recognized types and methods though. Think about just the type of a known object. Inliner could see that a certain object is an instance of a particular type, and on that basis it could decide to inline a method of that type at a virtual call site. If later we could end up with an instance of an incompatible type instead, that could cause trouble. (I'm not sure off the top of my head whether we currently take advantage of the types of known objects in this way during inlining, but there isn't any good reason in principle why we shouldn't. And I think it's better to avoid justifying things based on the current as opposed to inherent limitations of an analysis.) And this even matters for primitives. We could have a known array with known immutable elements. Let's assume that the array won't get swapped out somehow, and that its elements really are immutable and will never be overwritten. In the presence of this array, a constant index into it is directly analogous to a constant reference: we can fold the index in such a way as to make the IL / resulting code insensitive to later changes to it. Inliner could fold an integer, and then fold an array load where it's used as the index, and use the resulting known object to decide what to inline. If that integer is later modified, we can have exactly the same kind of situation as above.
More generally, any constant observed during inlining might have been relevant to the determination of another constant that was used later to determine a call target. For example, I'm working on implementing branch folding in OpenJ9's inliner. One constant could cause the inliner to treat a branch target as dead, and something else could be constant specifically because we know which way the branch goes. If some change to the first constant could be observed (either at runtime, or later during the same compilation), then the code might not really be dead in the IL / at runtime, and that could undermine the second constant. Fundamentally this recording/replay of constants is about ensuring that when the inliner decides that something is constant, it doesn't turn out later to have been wrong simply because the rest of the compiler made a (generally allowed but) inconsistent decision. Rather, the rest of the compiler should follow through so that the decision from the inliner sticks. Finally, I want to say that ensuring this kind of consistency is a safe default. Even if we didn't have specific examples of problems that could arise, this is the kind of thing that IMO should be treated as potentially important unless there is a strong argument to the contrary. |
|
jenkins build win |
|
jenkins build riscv |
|
Ok, thanks for the answer. I understand the functional issues are around "a value might be allowed to be folded despite the possibility of a later change" now. Checks have passed except for known issues. So I am merging. |
If inlining uses constant folding to help determine call targets, then it's important for the corresponding IL (if any is generated) to be consistent with what the inliner saw.
This commit equips
TR_CallTargetwith a collection of observed constants that must be folded consistently in IL. This collection can be populated by an inliner and later consulted by an IL generator to ensure that the operations are folded as needed.This repeated folding is important whenever a value might be allowed to be folded despite the possibility of a later change. It also allows constants to be speculative by specifying the assumptions that are necessary in order for the folding to be correct, and by informing the IL generator of the locations where such assumptions have been made.
Additionally, define
TR::mapandTR::setanalogously toTR::vectorandTR::list.