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
AndyAyersMS merged 2 commits into from
Jun 16, 2022
Merged

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

Changes from 1 commit
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
Next Next commit
JIT: relax fwd sub restriction on changing class handle 
For HW SIMD types, at least.

Fixes #64879.
  • Loading branch information
@AndyAyersMS
AndyAyersMS committed Jun 10, 2022
commit 34bfc048bf24d74b2ade9382ec1b2addab45b844
25 changes: 16 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,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))
Copy link
Member

@tannergooding tannergooding Jun 10, 2022

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

@tannergooding tannergooding Jun 10, 2022

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

@tannergooding tannergooding Jun 10, 2022

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

@SingleAccretion SingleAccretion Jun 10, 2022

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

@tannergooding tannergooding Jun 10, 2022

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

@SingleAccretion SingleAccretion Jun 10, 2022

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

@AndyAyersMS AndyAyersMS Jun 10, 2022

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

@SingleAccretion SingleAccretion Jun 10, 2022

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

@jakobbotsch jakobbotsch Jun 10, 2022

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;
}

if (!isHWSIMDClass(fwdHnd) || !isHWSIMDClass(useHnd))
Copy link
Member

@tannergooding tannergooding Jun 10, 2022

Choose a reason for hiding this comment

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

What are the concerns around this for Vector<T> and Vector2/3/4 (which are the non-HW SIMD handle types)

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 10, 2022

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@SingleAccretion SingleAccretion 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.

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 a in an OBJ(ADDR), but there are some cases with temps where we don't do that)

Copy link
Member

@tannergooding tannergooding Jun 10, 2022

Choose a reason for hiding this comment

The 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 Vector4 reinterpreted as a Vector128 in import (the .AsVector128 is just dropped). So I would expect this to already be an issue...

Copy link
Contributor

@SingleAccretion SingleAccretion 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.

It is currently not an issue because impNormStructVal makes sure to either wrap things with an OBJ<correct handle>, or spill to a local of the correct type.

The sequence of events where things would "go wrong" would be along the lines of:

  1. We spill Vector4.Zero to a temp typed as Vector128<int>.
  2. We forward-substitute into said temp. (which cannot happen today)

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;
}
Copy link
Member

@jakobbotsch jakobbotsch Jun 10, 2022

Choose a reason for hiding this comment

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

What is the reason to disallow this in the first place?

Copy link
Contributor

@SingleAccretion SingleAccretion 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.

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 sizeof(LHS) != sizeof(RHS).

Copy link
Member

@jakobbotsch jakobbotsch Jun 10, 2022

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@SingleAccretion SingleAccretion 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.

How relevant is the first reason with recent call arg work?

It basically comes down to replacing this:

runtime/src/coreclr/jit/morph.cpp

Line 2350 in fae7ee8

objClass = comp->gtGetStructHandle(argx);

With objClass = arg.GetSignatureClassHandle.

(And also deleting some other oddities like InferOpSizeAlign using lvStructDoubleAlign, GetHfaType(argx), etc)

Copy link
Member

@tannergooding tannergooding Jun 10, 2022

Choose a reason for hiding this comment

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

there are also (now rare, or actually non-existent, I did not check) cases where block copies can end up with sizeof(LHS) != sizeof(RHS).

Could you elaborate on how this is impactful? For TYP_SIMD and any non-TYP_STRUCT, I'd expect we treat a block copy as just a raw copy of the bits and so the handle shouldn't matter.

Copy link
Contributor

@SingleAccretion SingleAccretion 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.

Could you elaborate on how this is impactful?

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 a with b could lead to all sorts of interesting things.

Copy link
Member

@tannergooding tannergooding Jun 10, 2022

Choose a reason for hiding this comment

The 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 varType should differ, let alone the class handle.

That is, TYP_SIMD8 shouldn't be substitutable for a TYP_SIMD16. Just TYP_SIMD16 for any other TYP_SIMD16, regardless of simdJitBaseType

Copy link
Contributor

@SingleAccretion SingleAccretion Jun 10, 2022

Choose a reason for hiding this comment

The 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 varType should differ

In my example they're both TYP_STRUCT, so that check would pass.

Copy link
Member

@tannergooding tannergooding Jun 10, 2022

Choose a reason for hiding this comment

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

I'd expect for TYP_STRUCT we have to check the handles and for that case to fail since they mismatch.

I'd expect that for TYP_STRUCT and TYP_REF that checking the handle is needed. I'd expect for most primitive types and simd types, just the var type is enough.

There may of course be nuance around the integer types since TYP_UINT often appears as a TYP_INT with "unsignedness" being tracked elsewhere.

Copy link
Contributor

@SingleAccretion SingleAccretion Jun 10, 2022

Choose a reason for hiding this comment

The 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 ASG. Currently, we need the handle checks for varTypeIsStruct nodes for the reasons outlined above.

}

#ifdef FEATURE_SIMD
Expand Down