Introduce new CORJIT_FLAGS type #7837

BruceForstall · 2016-10-26T23:01:22Z

The "JIT flags" currently passed between the EE and the JIT have traditionally
been bit flags in a 32-bit word. Recently, a second 32-bit word was added to
accommodate additional flags, but that set of flags is definitely "2nd class":
they are not universally passed, and require using a separate set of bit
definitions, and comparing those bits against the proper, 2nd word.

This change replaces all uses of bare DWORD or 'unsigned int' types
representing flags with CORJIT_FLAGS, which is now an opaque type. All
flag names were renamed from CORJIT_FLG_* to CORJIT_FLAG_* to ensure all
cases were changed to use the new names, which are also scoped within the
CORJIT_FLAGS type itself.

Another motivation to do this, besides cleaner code, is to allow enabling the
SSE/AVX flags for x86. For x86, we had fewer bits available in the "first
word", so would have to either put them in the "second word", which, as
stated, was very much 2nd class and not plumbed through many usages, or
we could move other bits to the "second word", with the same issues. Neither
was a good option.

RyuJIT compiles with both COR_JIT_EE_VERSION > 460 and <= 460. I introduced
a JitFlags adapter class in jitee.h to handle both JIT flag types. All JIT
code uses this JitFlags type, which operates identically to the new
CORJIT_FLAGS type.

In addition to introducing the new CORJIT_FLAGS type, the SSE/AVX flags are
enabled for x86.

The JIT-EE interface GUID is changed, as this is a breaking change.

BruceForstall · 2016-10-26T23:07:02Z

@dotnet/jit-contrib PTAL
@jkotas @janvorli Can one of you look at the VM changes, or nominate someone to do so?

Things to look at: (1) corjit.h, which defines the new CORJIT_FLAGS, (2) jitee.h which defines an adapter class for the JIT usage of CORJIT_FLAGS, (3) VM: cgenx86.cpp for xmmYmmStateSupport(), codeman.cpp -- EEJitManager::SetCpuInfo() for enabling SSE/AVX flag checking/setting for x86.

CarolEidt · 2016-10-27T00:22:32Z

So, if I understand correctly, you haven't eliminated any flags or added any new ones; you've just made the additional SSE/AVX flags available for X86, moved some actual values around and abstracted the interface. Is that right?

janvorli · 2016-10-27T00:25:02Z

src/vm/i386/cgenx86.cpp

 }

+extern "C"
+__declspec(naked)


What is the reason for making this function naked?

I'm not well-versed in how __asm works. I copy-and-pasted the code from the amd64 asm file, and making it naked allowed that to work. If I made it non-naked, what would I do? Remove the "ret" but leave everything else? I had a problem before where there were contracts getting in the way, too.

Yes, in the case of this function, removing the "ret" should be enough. Since the cpuid and getcpuid functions above are not naked and similar to this function, I was wondering why this one needs to be naked.

CarolEidt · 2016-10-27T00:27:46Z

src/jit/jitee.h

+
+// This class wraps the CORJIT_FLAGS type in the JIT-EE interface (in corjit.h) such that the JIT can
+// build with either the old flags (COR_JIT_EE_VERSION <= 460) or the new flags (COR_JIT_EE_VERSION > 460).
+// It actually is exactly the same as the new definition, and must be kept up-to-date with the new definition.


It seems quite unfortunate to have these two things that must be kept in sync. I'm having trouble understanding why this extra layer is needed. Can you articulate the motivation for me?

The issue is we must be able to build with either COR_JIT_EE_VERSION <= 460, and the old CORJIT_FLAGS type definition (a struct of 2 dwords and all the old CorJitFlag enum declarations) -- this is the CTP JIT -- or with COR_JIT_EE_VERSION > 460 and the new CORJIT_FLAGS type definition (my new 'class') and new enum values. Probably, after we ship the next desktop update, we can assume that any future CTP desktop JIT would run on top of that, and we can get rid of all COR_JIT_EE_VERSION #ifdefs and subsequent JIT-EE interface changes would be made as #if COR_JIT_EE_VERSION > 463 (for example). That would allow us to eliminate this adapter class if desired.

CarolEidt · 2016-10-27T01:17:02Z

src/jit/compiler.cpp

    {
-        assert((compileFlags & CORJIT_FLG_LOST_WHEN_INLINING) == 0);
-        assert(compileFlags & CORJIT_FLG_SKIP_VERIFICATION);
+        assert(!jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT));


These are now broken out separately here and in flowgraph.cpp. Would it make sense to add methods to JitFlags that set/clear and assert the appropriate flags for inlining? That way they would be in close proximity and more easily kept in sync.

That's possible, but I was hoping that JitFlags would be a thin adapter class, and this would add more functionality to it than exists in the CORJIT_FLAGS class. Another option would be to create a single "global" or static CORJIT_FLAGS object, say JitFlagsLostWhenInlining and initialize it to the set, then use the appropriate CORJIT_FLAGS Add/Remove functions on the entire set.

I don't feel strongly about it, but I think it might be cleaner

In reply to: 85267896 [](ancestors = 85267896)

mikedn · 2016-10-27T05:05:43Z

src/jit/codegencommon.cpp

-                            (compiler->compCodeOpt() != Compiler::SMALL_CODE) &&
-                                !(compiler->opts.eeFlags & CORJIT_FLG_PREJIT)
+                            , (compiler->compCodeOpt() != Compiler::SMALL_CODE) &&
+                                !compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT)


JitFlags isn't a scoped enum so JitFlags:: is not required. And if you make it a scoped enum then the JIT_FLAG_ prefix should go away.

Good point. I like the scoped idea better.

BruceForstall · 2016-10-27T05:15:50Z

So, if I understand correctly, you haven't eliminated any flags or added any new ones; you've just made the additional SSE/AVX flags available for X86, moved some actual values around and abstracted the interface. Is that right?

Yes, that is correct. Actually, I didn't really even move any around; I enabled the SSE/AVX flags for x86, which pushed other flags out to larger indices. Because the interface is abstracted, no clients need to know if the "pushed out" flags are in (what was) dword 1 or dword 2.

mikedn · 2016-10-27T09:06:47Z

src/vm/i386/cgenx86.cpp

+        xgetbv                          ; result in EDX:EAX
+        and eax, 06H
+        cmp eax, 06H                    ; check OS has enabled both XMM and YMM state support
+        jne     not_supported


Not that it would matter performance wise but this is just

sete al

CarolEidt · 2016-10-27T14:40:39Z

The "JIT flags" currently passed between the EE and the JIT have traditionally been bit flags in a 32-bit word. Recently, a second 32-bit word was added to accommodate additional flags, but that set of flags is definitely "2nd class": they are not universally passed, and require using a separate set of bit definitions, and comparing those bits against the proper, 2nd word. This change replaces all uses of bare DWORD or 'unsigned int' types representing flags with CORJIT_FLAGS, which is now an opaque type. All flag names were renamed from CORJIT_FLG_* to CORJIT_FLAG_* to ensure all cases were changed to use the new names, which are also scoped within the CORJIT_FLAGS type itself. Another motivation to do this, besides cleaner code, is to allow enabling the SSE/AVX flags for x86. For x86, we had fewer bits available in the "first word", so would have to either put them in the "second word", which, as stated, was very much 2nd class and not plumbed through many usages, or we could move other bits to the "second word", with the same issues. Neither was a good option. RyuJIT compiles with both COR_JIT_EE_VERSION > 460 and <= 460. I introduced a JitFlags adapter class in jitee.h to handle both JIT flag types. All JIT code uses this JitFlags type, which operates identically to the new CORJIT_FLAGS type. In addition to introducing the new CORJIT_FLAGS type, the SSE/AVX flags are enabled for x86. The JIT-EE interface GUID is changed, as this is a breaking change.

briansull · 2016-10-27T20:26:37Z

LGTM

Fixes dotnet#7837 The full description of the problem is in that issue.

Fixes #7837 The full description of the problem is in that issue.

dnfclas added the cla-already-signed label Oct 26, 2016

janvorli reviewed Oct 27, 2016

View reviewed changes

CarolEidt reviewed Oct 27, 2016

View reviewed changes

mikedn reviewed Oct 27, 2016

View reviewed changes

MichalStrehovsky mentioned this pull request Oct 27, 2016

Implement initThisClass for generic types in ReadyToRun #7842

Merged

BruceForstall force-pushed the JitFlags4 branch from 01c65a8 to e72536c Compare October 27, 2016 19:14

BruceForstall merged commit 7250e6f into dotnet:master Oct 27, 2016

BruceForstall deleted the JitFlags4 branch October 27, 2016 20:48

AustinWise mentioned this pull request Oct 27, 2016

Fix conversion from CorJitFlag2 enum to JitFlag. #7857

Merged

BruceForstall added a commit to BruceForstall/coreclr that referenced this pull request Oct 30, 2016

Work around a Visual C++ silent bad code generation bug

143c73d

Fixes dotnet#7837 The full description of the problem is in that issue.

BruceForstall mentioned this pull request Oct 30, 2016

Work around a Visual C++ silent bad code generation bug #7890

Merged

jkotas pushed a commit that referenced this pull request Oct 30, 2016

Work around a Visual C++ silent bad code generation bug (#7890)

3fc65f3

Fixes #7837 The full description of the problem is in that issue.

karelz modified the milestone: 2.0.0 Aug 28, 2017

michellemcdaniel mentioned this pull request Jan 31, 2020

x86: Fix release failures introduced by #7837 dotnet/runtime#6904

Closed

Introduce new CORJIT_FLAGS type #7837

Introduce new CORJIT_FLAGS type #7837

Uh oh!

Conversation

BruceForstall commented Oct 26, 2016

Uh oh!

BruceForstall commented Oct 26, 2016

Uh oh!

CarolEidt commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt commented Oct 27, 2016

Uh oh!

briansull commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants