-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: relax fwd sub restriction on changing class handle #70587
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -504,12 +504,6 @@ bool Compiler::fgForwardSubStatement(Statement* stmt) | |||
return false; | ||||
} | ||||
|
||||
if (gtGetStructHandleIfPresent(fwdSubNode) != gtGetStructHandleIfPresent(lhsNode)) | ||||
{ | ||||
JITDUMP(" would change struct handle (assignment)\n"); | ||||
return false; | ||||
} | ||||
|
||||
// If lhs is mulit-reg, rhs must be too. | ||||
// | ||||
if (lhsNode->IsMultiRegNode() && !fwdSubNode->IsMultiRegNode()) | ||||
|
@@ -667,10 +661,23 @@ bool Compiler::fgForwardSubStatement(Statement* stmt) | |||
// | ||||
// We may sometimes lose or change a type handle. Avoid substituting if so. | ||||
// | ||||
if (gtGetStructHandleIfPresent(fwdSubNode) != gtGetStructHandleIfPresent(fsv.GetNode())) | ||||
// However, we allow free substitution of hardware SIMD types. | ||||
// | ||||
CORINFO_CLASS_HANDLE fwdHnd = gtGetStructHandleIfPresent(fwdSubNode); | ||||
CORINFO_CLASS_HANDLE useHnd = gtGetStructHandleIfPresent(fsv.GetNode()); | ||||
if (fwdHnd != useHnd) | ||||
{ | ||||
JITDUMP(" would change struct handle (substitution)\n"); | ||||
return false; | ||||
if ((fwdHnd == NO_CLASS_HANDLE) || (useHnd == NO_CLASS_HANDLE)) | ||||
{ | ||||
JITDUMP(" would add/remove struct handle (substitution)\n"); | ||||
return false; | ||||
} | ||||
|
||||
if (!isHWSIMDClass(fwdHnd) || !isHWSIMDClass(useHnd)) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What are the concerns around this for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing particular. We can do this for all SIMD types if that makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would not be correct because of the aforementioned call arguments issue. Case of interest: Vector128<int> a = Vector4.Zero;
Call(a); (Note that that particular example copied verbatim would be ok, as we would wrap There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SingleAccretion can you build a repro for this using: Vector128<int> a = Vector4.Zero.AsVector128();
Call(a); This will, today, have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is currently not an issue because The sequence of events where things would "go wrong" would be along the lines of:
It is not a theoretical problem. I've hit a case (#69965) that was similar in spirit (only there the handle pre-morph was correct, but lost after morphing the argument). |
||||
{ | ||||
JITDUMP(" would change struct handle (substitution)\n"); | ||||
return false; | ||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason to disallow this in the first place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if Andy ran into some other issues when bringing up forward sub, but the primary reason for exact handle match I would think of is to avoid substituting mismatched things into call arguments. This is also the reason we cannot allow more SIMD types here. Edit: there are also (now rare, or actually non-existent, I did not check) cases where block copies can end up with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How relevant is the first reason with recent call arg work? I would assume we're not far off from being able to let call args morphing handle this on its own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It basically comes down to replacing this: runtime/src/coreclr/jit/morph.cpp Line 2350 in fae7ee8
With (And also deleting some other oddities like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on how this is impactful? For There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Imagine we have a mismatched copy like this (we definitely used to have those before #69271, I am not sure if some cases still exist): struct<16> a = struct<8> b;
Use(a); Substituting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect that one to not get a forward sub since the That is, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In my example they're both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect for I'd expect that for There may of course be nuance around the integer types since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the "ideal future", we will need no handle checks whatsoever, because the "types" would implicitly match at the point of the |
||||
} | ||||
|
||||
#ifdef FEATURE_SIMD | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that if we have a
TYP_SIMD16
without a handle (such as because its a purely synthesized node) we can't substitute?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Happy to just broaden this and check for simd type, rather than class handles, if that's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of anything that would impact this. We have logic for eliding
AsVector4
andAsVector128
in import. Same forVector<T>
<->Vector128
orVector256
(which ever matches the size ofVector<T>
)This can basically be summed as:
TYP_SIMD8
can be subbed for anotherTYP_SIMD8
TYP_SIMD12
can be subbed for anotherTYP_SIMD12
TYP_SIMD16
can be subbed for anotherTYP_SIMD16
TYP_SIMD32
can be subbed for anotherTYP_SIMD32
If the
varType
doesn't match up, then substitution is invalid.I would generally expect we could do something like this
Where for "well known types", we can just bypass the handle check entirely since they are primitives or SIMD. But for unknown struct types and other special scenarios, we need to do a handle check.
I'm unsure if integers are a problem for certain cases like where
TYP_UINT
is tracked on the stack as aTYP_INT
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, this is assuming there aren't any "gotchas" like @SingleAccretion was mentioning above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As below and above, the call arguments issue prevents us from being permissive here. We cannot lose precise handles for them if they're
varTypeIsStruct
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds dangerous/complicated. I wouldn't expect something like
struct { byte x; }
to be subtitutable with say astruct { int x; }
.I'm guessing there is something I'm missing here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could restrict this pessimization to calls/multi-reg returns only.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at forward sub this handle check is just one of many cases where it won't substitute even if it should be legal and profitable (and likewise the legality and profitability analyses themselves can be improved).
Would be great if we could chip away at all these. I likely won't have much time to do this anytime soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ultimate goal is to make
ClassLayout::AreCompatible
the "type identity" for structs, and that would only allow same-sized structs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, yes, "mismatching class handles" was a bit too liberally phrased.