Currently if gc value used in deopt section is not mentioned in gc section
its lowering requires spill.
The patch introduces the option to allow lowering such value on register.
Option is off by default, so current behavior is not changed.
Details
Details
- Reviewers
reames dantrushin
Diff Detail
Diff Detail
Event Timeline
Comment Actions
ok, if we do not need to limit deopt gc values then I decided to return back almost to what you reverted.
Comment Actions
I don't believe this is safe to land as is, at least without some careful framing and documenting of assumptions.
The problem I see is that gc values are expected to have base pointers. A gc pointer which appears in the deopt list, but not in the gc-value list doesn't have an associated base. It might happen to be the base itself, but it also might not. Now, it may be that in some particular use case, we know any deopt value must be a base, but we need a way to encode that fact if so.
Taking a step back, how does this case arise? If this is truly dead code, we might be better off lowering as a poison constant (or simply dropping it).
Comment Actions
Hello Philip, thank you for your comment.
This corresponds to the case when some gc pointer is alive in deopt bundle while callee works and dead after return.
The typical case would be @llvm.experimental.deoptimize intrinsic. All gc values in deopt bundle are dead after the call.
RS4GC will generate the gc value mentioned in deopt bundle in gc bundle as well. Moreover it will generate a gc.relocate intruction for it.
However gc.relocate instruction will be removed by any DCE due to it has no uses (all code after deopt is dead) and trivially dead (as readonly).
After all gc.relocates are eliminated InstCombine will be able to remove this gc pointer from gc section.
As a result we come to the case when there is a deopt gc value which is not listed in gc section.
I could fix the instcombine to preserve the value in gc section but it will not help in terms of base/derived because the only place where base/derived
info is stored is gc.relocate.
If we really want to keep this information we should preserve gc.relocates for values listed in deopt bundle or find another place to track the property.
Comment Actions
Serguei,
From your comment, it sounds like we're already assuming values in the deopt list are base pointers fairly widely. (e.g. even the current stack lowering makes this assumption) Your explanation matches my vague memory, so I don't have any reason to question that.
With that in mind, I think it's okay to continue with that assumption, but I'd like to make that assumption as explicit as we can manage. (And document it explicitly!)
Glancing at the code, I want to suggest an alternate approach. I won't require this, but I'm curious to hear what you think.
At the MARKed location, what if we inserted something along the lines of the following:
// If we find a deopt value which isn't explicitly added, we need to
// ensure it gets lowered such that gc cycles occurring before the
// deoptimization event during the lifetime of the call don't invalidate
// the pointer we're deopting with. Note that we assume that all
// pointers passed to deopt are base pointers; relaxing that assumption
// would require relatively large changes to how we represent relocations.
for (Value *V : I.deopt_operands()) {
if (!isGCValue(V)) continue;
if (Seen.insert(V).second) {
SI.Bases.push_back(V);
SI.Ptrs.push_back(V);
}
}I think this has the same effect, but is much more explicit about what is going on.
Your take?
| llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp | ||
|---|---|---|
| 1067 | MARK (see overall comment) | |
Comment Actions
Philip, I've published alternative patch https://reviews.llvm.org/D97554 wit your idea to show the impact.
From my point of view it looks better in terms that we explicitly process deopt gc pointers as other gc values.
The disadvantages is that now we will have a lot of dead tied-def registers and deopt value will be listed twice in uses.
So generally the approach you proposed is a bit cleaner in understanding but costs a bot more in terms of compile time.
Comment Actions
This comment confused me greatly, and caused me to take a closer look at the current patch. After doing so, I'm glad I did! This version is unsound.
For the gc pointer to remain valid until deoptimization, we must update it during any GC cycles during the lifetime of the call and before the deopt point. To do that, we need to model the call as modifying the pointer. Otherwise, the register allocator is free to place it in a read only location (such as an argument slot) or combine the updated and non-updated copies of value into a single register. (Consider the case where a null check on the original unrelocated value was reordered after the safepoint.) The fact this one doesn't cause a tied def is a symptom of a bug, not a code quality improvement.
Comment Actions
I agree that alternative solution is better in terms of probability but here we need fixup caller saved registers anyway.
In alternative approach we end up with something like
tied-def %1 = statepoint (deopt %1) (gc tied-use %1)
ad in theory nothing prevents RA to allocate different registers for use in gc and use in deopt. It is unlikely but anyway.
This is just a comment.
Seems you don't need a map, set would suffice here. Which would greatly simplify code.