VC++ truncates X86II::VEX enum constants

[DimitryAndric]

Bugzilla Link	7748
Resolution	FIXED
Resolved on	Aug 25, 2010 20:04
Version	trunk
OS	Windows XP
CC	@bcardosolopes

Extended Description

During compilation with VC++ 9 of several llvm files that include
X86InstInfo.h, I am getting the following warnings about it:

lib\target\x86\X86InstrInfo.h(442) : warning C4341: 'VEX' : signed value is out of range for enum constant
lib\target\x86\X86InstrInfo.h(442) : warning C4309: 'initializing' : truncation of constant value
lib\target\x86\X86InstrInfo.h(446) : warning C4341: 'VEX_W' : signed value is out of range for enum constant
lib\target\x86\X86InstrInfo.h(446) : warning C4309: 'initializing' : truncation of constant value
lib\target\x86\X86InstrInfo.h(451) : warning C4341: 'VEX_4V' : signed value is out of range for enum constant
lib\target\x86\X86InstrInfo.h(451) : warning C4309: 'initializing' : truncation of constant value
lib\target\x86\X86InstrInfo.h(456) : warning C4341: 'VEX_I8IMM' : signed value is out of range for enum constant
lib\target\x86\X86InstrInfo.h(456) : warning C4309: 'initializing' : truncation of constant value
lib\target\x86\X86InstrInfo.h(463) : warning C4341: 'VEX_L' : signed value is out of range for enum constant
lib\target\x86\X86InstrInfo.h(463) : warning C4309: 'initializing' : truncation of constant value

This is because of the following enum constants in the X86II namespace:

// VEX - The opcode prefix used by AVX instructions
VEX         = 1ULL << 32,

// VEX_W - Has a opcode specific functionality, but is used in the same
// way as REX_W is for regular SSE instructions.
VEX_W       = 1ULL << 33,

// VEX_4V - Used to specify an additional AVX/SSE register. Several 2
// address instructions in SSE are represented as 3 address ones in AVX
// and the additional register is encoded in VEX_VVVV prefix.
VEX_4V      = 1ULL << 34,

// VEX_I8IMM - Specifies that the last register used in a AVX instruction,
// must be encoded in the i8 immediate field. This usually happens in
// instructions with 4 operands.
VEX_I8IMM   = 1ULL << 35,

// VEX_L - Stands for a bit in the VEX opcode prefix meaning the current
// instruction uses 256-bit wide registers. This is usually auto detected if
// a VR256 register is used, but some AVX instructions also have this field
// marked when using a f256 memory references.
VEX_L       = 1ULL << 36

So unfortunately VC++ truncates these constants, which is probably very
bad if you count on those exact values. The MSDN library says the
following about this warning:

"An enumerated constant exceeds the limit for an int. The value of the
invalid constant is undefined. Constants must resolve to integers
between –4,294,967,295 and +4,294,967,295 (signed)."

http://msdn.microsoft.com/en-us/library/x651y302.aspx

Therefore, these constants should probably be replaced by globals, or
maybe even #defines... (yuck :)

[bcardosolopes]

That's bad. Not going use #define's for those though. If any other better solution doesn't show up, we should go for putting VEX enums in a new enum, and shift TSFlags before checking.

[DimitryAndric]

Fix VEX enum constant overflow
This seems to be a relatively easy way of fixing these enum constant
overflows. Just define the constants as shift counts instead, and use
"TSFlags & (1ULL << VEX_XXX_Shift)" in the appropriate places.

[bcardosolopes]

That's an option, another option is: "(TSFlags >> 32) & VEX_XXX", considering that VEX_XXX enums would start from 0 again

[DimitryAndric]

Alternative fix for VEX enum constant overflow
Sure, here is a patch for the >>32 alternative.

[bcardosolopes]

Created an attachment (id=5287) [details]
Alternative fix for VEX enum constant overflow

Sure, here is a patch for the >>32 alternative.

The patch seems fine, but breaks one test/CodeGen/X86 test, can you improve the patch to successfully pass all those?

Thanks.

[DimitryAndric]

I would really want to fix that test, but it seems a great lot of tests
simply do not work correctly on Windows yet.

Can you please point out the specific test that is failing?

[llvmbot]

Use C#-style enum type extension
There's actually a semi-obscure Microsoft extension to make the enum explicitly 64-bit; just change "enum {" to "enum : unsigned __int64 {" when compiling on MSVC:

http://msdn.microsoft.com/en-us/library/2dzy4k6e(VS.80).aspx
http://msdn.microsoft.com/en-us/library/ms173702.aspx

[bcardosolopes]

I'm not really sure of what approach should be done here. I'll wait for more people to see this.

[bcardosolopes]

Dimitry,

Check your patch and re-send it and I'll give it a try again. Just make sure all places properly uses TSFlags, this commit initially changed it, may it helps you:
http://llvm.org/viewvc/llvm-project?rev=107952&view=rev

Sorry for the delay,
Thanks

[bcardosolopes]

You actually just missed one TSFlags and I've foreseen! I fixed it myself and commited in r112128!
Thanks!