Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

IsSupported returns false for not-fully-implemented ISA classes #15514

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

fiigii
Copy link

@fiigii fiigii commented Dec 13, 2017

Resolve #14930

@fiigii
Copy link
Author

fiigii commented Dec 13, 2017

@jkotas @CarolEidt @BruceForstall My current solution is to add a new environment variable COMPlus_EnableIncompleteISAClass to test partially implemented classes.

Copy link

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

I think the new config variable is a good approach, but would like to see it enabled only under DEBUG (i.e. Debug or Checked builds).

case InstructionSet_FMA:
case InstructionSet_PCLMULQDQ:
// DONE - InstructionSet_LZCNT:
// DONE - InstructionSet_POPCNT:

Choose a reason for hiding this comment

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

I would prefer to see explicit cases for the ones that are done, and then the default should be unreached().

if ((!compSupports(isa) || (!featureSIMD && isa != InstructionSet_BMI1 && isa != InstructionSet_BMI2 &&
isa != InstructionSet_LZCNT && isa != InstructionSet_POPCNT)) &&
// - the ISA class is not yet fully implemented and EnableIncompleteISAClass=false
if ((!compSupports(isa) || (!JitConfig.EnableIncompleteISAClass() && !isFullyImplmentedISAClass(isa)) ||

Choose a reason for hiding this comment

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

I think it would be cleaner to have this check look something like this:

if ((!compSupports(isa) || !compSupportsIntrinsics(isa)) && !isIntrinsicAnIsSupportedPropertyGetter(intrinsic))

Then, compSupportsIntrinsics(isa) can incorporate both the check for those that are fully supported, as well as, under DEBUG only, checking JitConfig.EnableIncompleteISAClass().

Copy link
Author

Choose a reason for hiding this comment

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

That would be more elegant. Will change.

@@ -186,6 +186,33 @@ bool Compiler::isIntrinsicAnIsSupportedPropertyGetter(NamedIntrinsic intrinsic)
}
}

// TODO - remove the fully implemented ISA

Choose a reason for hiding this comment

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

I think that all these TODO comments should be omitted. I expect that it will take some time to complete the current set of ISAs, and there are likely to be more in future.

@fiigii
Copy link
Author

fiigii commented Dec 14, 2017

I think the new config variable is a good approach, but would like to see it enabled only under DEBUG

@CarolEidt I remember we talked this issue in that email thread. @jkotas mentioned that inconsistent behavior between release and debug build will make some test problems.
If the inconsistency is allowed, I think this new envirnoment variable would be unnecessary.
My original plan was to disable incomplete classes in release and debug both, then add some new CI jobs with COMPlus_EnableIncompleteISAClass.

@fiigii
Copy link
Author

fiigii commented Dec 14, 2017

@jkotas @BruceForstall comments?

@jkotas
Copy link
Member

jkotas commented Dec 14, 2017

Without any environment variable, the debug and release need to behave the same.

It is fine for the environment variable to be respected in debug build only. Debug and release can behave differently with environment variable set.

@fiigii
Copy link
Author

fiigii commented Dec 14, 2017

It is fine for the environment variable to be respected in debug build only. Debug and release can behave differently with environment variable set.

Got it, thank you!

@fiigii
Copy link
Author

fiigii commented Dec 17, 2017

Moved COMPlus_EnableIncompleteISAClass under DEBUG and addressed feedback. @jkotas @CarolEidt PTAL.

@@ -3044,6 +3044,7 @@ class Compiler
GenTree* impLZCNTIntrinsic(NamedIntrinsic intrinsic, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* sig);
GenTree* impPCLMULQDQIntrinsic(NamedIntrinsic intrinsic, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* sig);
GenTree* impPOPCNTIntrinsic(NamedIntrinsic intrinsic, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* sig);
bool compSupportHWIntrinsic(InstructionSet isa);

Choose a reason for hiding this comment

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

This should be compSupportsHWIntrinsic

// TODO - remove "JitConfig.EnableIncompleteISAClass()" after fully implement hardware intrinsics of this
// class
retNode =
gtNewIconNode(JitConfig.EnableIncompleteISAClass() && featureSIMD && compSupports(InstructionSet_SSE));

Choose a reason for hiding this comment

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

Isn't this test redundant with the one above? The check for JitConfig.EnableIncompleteISAClass() can only be done under DEBUG, but I believe that this check is redundant anyway, unless there's something I've missed.
Also, I still think these "TODO" comments are unnecessary, as I believe that we will need that facility for some time as new ISA feature sets are added and implemented over time.

Copy link
Author

Choose a reason for hiding this comment

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

Isn't this test redundant with the one above?

No, this check lets IsSupported return false for incomplete classes, but the above one lets intrinsics throw PNSE when the call site is not guarded by IsSupported.

The check for JitConfig.EnableIncompleteISAClass() can only be done under DEBUG,

Oops, my mistake. Will fix.

isa != InstructionSet_LZCNT && isa != InstructionSet_POPCNT)) &&
!isIntrinsicAnIsSupportedPropertyGetter(intrinsic))
// - JIT does not support this hardware intrinsics (compSupportHWIntrinsic return false)
if ((!compSupports(isa) || !compSupportHWIntrinsic(isa)) && !isIntrinsicAnIsSupportedPropertyGetter(intrinsic))

Choose a reason for hiding this comment

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

Could you extract the check for the specific ISA intrinsics at the beginning, e.g.

bool supported = (featureSIMD && compSupports(isa) && compSupportsHWIntrinsics(isa));

Then, the above check can be:

if (!supported && !isIntrinsicAnIsSupportedPropertyGetter(intrinsic))

and the checks below can be:

retNode = gtNewIconNode(supported);

Copy link
Author

Choose a reason for hiding this comment

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

Good point, will do.

@fiigii
Copy link
Author

fiigii commented Dec 19, 2017

Streamlined IsSupported code. @CarolEidt @jkotas PTAL

@@ -186,6 +186,49 @@ bool Compiler::isIntrinsicAnIsSupportedPropertyGetter(NamedIntrinsic intrinsic)
}
}

bool isFullyImplmentedISAClass(InstructionSet isa)
Copy link
Member

Choose a reason for hiding this comment

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

This function needs a standard header comment

Copy link
Member

Choose a reason for hiding this comment

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

Also, please make this a (static) member of the Compiler class.

}
}

// return true if
Copy link
Member

Choose a reason for hiding this comment

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

Please use a standard header comment format

// - isa is fully implemented or EnableIncompleteISAClass=true
bool Compiler::compSupportsHWIntrinsic(InstructionSet isa)
{
return (featureSIMD || isa == InstructionSet_BMI1 || isa == InstructionSet_BMI2 || isa == InstructionSet_LZCNT ||
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you should add a:

bool Compiler::compIsScalarISA(InstructionSet isa)
{
  return (isa == InstructionSet_BMI1) || (isa == InstructionSet_BMI2) || (isa == InstructionSet_LZCNT) || (isa == InstructionSet_POPCNT);
}

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, will change.

@fiigii fiigii force-pushed the 14930 branch 2 times, most recently from f94a3c9 to 58ba214 Compare December 20, 2017 01:35
@fiigii
Copy link
Author

fiigii commented Dec 20, 2017

@BruceForstall addressed feedback. PTAL.

@danmoseley
Copy link
Member

FYI @eerhardt

@fiigii
Copy link
Author

fiigii commented Dec 20, 2017

@dotnet-bot test Windows_NT x86 Checked Innerloop Build and Test

@fiigii
Copy link
Author

fiigii commented Dec 21, 2017

@CarolEidt @BruceForstall ping?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants