Skip to content

JIT: relax fwd sub restriction on changing class handle #70587

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 16, 2022
Merged

JIT: relax fwd sub restriction on changing class handle #70587

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
34bfc04
JIT: relax fwd sub restriction on changing class handle
AndyAyersMS Jun 10, 2022
4826a7f
fix build issue
AndyAyersMS Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/coreclr/jit/forwardsub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -667,10 +661,29 @@ 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))
Copy link
Member

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?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 10, 2022 •
edited
Loading

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.

Copy link
Member

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 and AsVector128 in import. Same for Vector<T> <-> Vector128 or Vector256 (which ever matches the size of Vector<T>)

This can basically be summed as:

  • TYP_SIMD8 can be subbed for another TYP_SIMD8
  • TYP_SIMD12 can be subbed for another TYP_SIMD12
  • TYP_SIMD16 can be subbed for another TYP_SIMD16
  • TYP_SIMD32 can be subbed for another TYP_SIMD32

If the varType doesn't match up, then substitution is invalid.

I would generally expect we could do something like this

if (varType1 != varType2)
{
    // mismatching varTypes, can't subsitute
    return false;
}

if (varTypeIsArithmetic(varType1) || varTypeIsSIMD(varType1))
{
    // We have a primitive arithmetic varType, like int, bool, short, etc (does TYP_INT vs TYP_UINT matter since we lose that sometimes?)
    // or one of the basic TYP_SIMD types and so substitution is possible and trivial
    return true;
}

// Now fallback to handle checking for other struct types, for byrefs, or other special considerations

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 a TYP_INT.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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 a struct { int x; }.

I'm guessing there is something I'm missing here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that means we'd need to special case forward substitutions when the use is a call (must always check handle), but otherwise it "should" be fine?

Yes, we could restrict this pessimization to calls/multi-reg returns only.

And once we fix morph to use the arg info directly, then we could change it (potentially) to be something more like what I was suggesting where we only compare the handles for unknown struct types?

Yes.

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect something like struct { byte x; } to be subtitutable with say a struct { int x; }.

The ultimate goal is to make ClassLayout::AreCompatible the "type identity" for structs, and that would only allow same-sized structs.

Copy link
Member

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.

{
JITDUMP(" would add/remove struct handle (substitution)\n");
return false;
}

#ifdef FEATURE_SIMD
const bool bothHWSIMD = isHWSIMDClass(fwdHnd) && isHWSIMDClass(useHnd);
#else
const bool bothHWSIMD = false;
#endif

if (!bothHWSIMD)
{
JITDUMP(" would change struct handle (substitution)\n");
return false;
}
}

#ifdef FEATURE_SIMD
Expand Down