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

Fix x86 encoder to use 64-bit type to accumulate opcode/prefix bits #8382

Merged
merged 1 commit into from
Dec 1, 2016

Conversation

BruceForstall
Copy link
Member

No description provided.

@BruceForstall
Copy link
Member Author

@sivarv PTAL
cc @dotnet/jit-contrib

@@ -28,6 +28,16 @@ inline static bool isDoubleReg(regNumber reg)
/* Routines that compute the size of / encode instructions */
/************************************************************************/

// code_t is a type used to accumulate bits of opcode + prefixes. On amd64,
// it must be 64 bits to the REX prefixes. On both x86 and amd64, it must be
Copy link
Member

Choose a reason for hiding this comment

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

Typo: Probably you want to mean

"On amd64 it must be 64-bits to support REX prefixes"

typedef size_t code_t;
#else // !defined(LEGACY_BACKEND)
typedef unsigned __int64 code_t;
#endif // !defined(LEGACY_BACKEND)
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking whether the right #ifdef here should be FEATURE_AVX_SUPPORT instead of LEGACY_BACKEND.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it, although currently FEATURE_AVX_SUPPORT == !LEGACY_BACKEND for xarch.

#endif // LEGACY_BACKEND
}

// Utility routines that abstract the logic of adding REX.W, REX.R, REX.X, REX.B and REX prefixes
// SSE2: separate 1-byte prefix gets added before opcode.
// AVX: specific bits within VEX prefix need to be set in bit-inverted form.
size_t emitter::AddRexWPrefix(instruction ins, size_t code)
emitter::code_t emitter::AddRexWPrefix(instruction ins, code_t code)
Copy link
Member

Choose a reason for hiding this comment

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

emitter::code_t [](start = 0, length = 15)

Can't we use code_t here instead of emitter::code_t?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope; the compiler requires the class prefix for the return value.

{
#ifdef _TARGET_AMD64_ // TODO-x86: This needs to be enabled for AVX support on x86.
#ifdef FEATURE_AVX_SUPPORT
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this #ifdef since hasVexPrefix() returns false if FEATURE_AVX_SUPPORT is not defined.

else if (code > 0x00FFFFFFFFLL)
#endif // FEATURE_AVX_SUPPORT

#ifdef _TARGET_AMD64_
Copy link
Member

Choose a reason for hiding this comment

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

I thought this logic is needed for emitting 3/4 byte SSE instructions. Isn't this required for x86 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The REX prefixes are only used for AMD64. When set in the AVX prefix, they are only used for extended register usage.

#endif
if (code & REX_PREFIX_MASK)

#ifdef _TARGET_AMD64_
Copy link
Member

Choose a reason for hiding this comment

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

#ifdef _TARGET_AMD64 [](start = 0, length = 20)

Isn't this required for x86 ryujit as well?

Copy link
Member

Choose a reason for hiding this comment

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

I see call to this method is under amd64. But we might have to change it for x86 ryujit given SSE2 is require?


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

@sivarv
Copy link
Member

sivarv commented Nov 30, 2016

:shipit:

@sivarv
Copy link
Member

sivarv commented Nov 30, 2016

Looks good.

The encoder was using size_t, a 32-bit type on x86, to accumulate opcode
and prefix bits to emit. AVX support uses 3 bytes for prefixes that are
higher than the 32-bit type can handle. So, change all code byte related types
from size_t to a new code_t, defined as "unsigned __int64" on RyuJIT x86
(there is precedence for this type on the ARM architectures).

Fixes #8331
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.

LGTM

@BruceForstall BruceForstall merged commit 392748e into dotnet:master Dec 1, 2016
@BruceForstall BruceForstall deleted the FixShift branch December 1, 2016 00:47
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fix x86 encoder to use 64-bit type to accumulate opcode/prefix bits

Commit migrated from dotnet/coreclr@392748e
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.

5 participants