[PATCH] D27901: [X86][[AVX512] Code size reduction in X86 by replacing EVEX with VEX encoding

[Gadi Haber via Phabricator via llvm-commits]

gadi.haber marked 7 inline comments as done.
gadi.haber added inline comments.


================
Comment at: lib/Target/X86/InstPrinter/X86InstComments.h:20
+
+  enum AsmComments {
+    AC_EXEX_2_VEX = 0x2 // for instr that was compressed from EVEX to VEX
----------------
craig.topper wrote:
> The other bit (bit 0) used for AsmComments is target independent. How do we keep someone from adding a new target independent bit 1 and conflicting with this usage? We should have some documentation and probably an enum contant in MachineInstr.h that indicates which bits can be used for target dependent things.
I'm open to suggestions on this.

One option is to simply add the following  comment in MachineInstr.h:

enum CommentFlag {
    ReloadReuse = 0x1 // higher bits are reserved for target dep comments.
  };


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:151
+    // check for EVEX instructions only
+    if (!(Desc.TSFlags & X86II::EVEX))
+      continue;
----------------
craig.topper wrote:
> This should be (Desc.TSFlags & X86II::EncodingMask) == X86II::EVEX)
> 
> X86II::EVEX is not a bit mask its a value for the encoding field.
Good catch. Thanx!
You probably meant: 
(Desc.TSFlags & X86II::EncodingMask) != X86II::EVEX



================
Comment at: lib/Target/X86/X86EvexToVex.cpp:168
+
+    // EVEX_V128 instr: bit EVEX_L2 = 0, bit VEX_L = 0
+    bool isEVEX_V128 = (!(Desc.TSFlags & X86II::EVEX_L2) && 
----------------
craig.topper wrote:
> What do we get out of having two different DenseMaps? Couldn't we have 128 and 256 in the same map?
The smaller the DenseMap table is the faster the find() method returns a value. It is therefore, recommended to chunk the DenseMap table to smaller tables whenever possible. 


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:206
+      
+      if (X86::VR256RegClass.contains(Reg) ||
+          X86::VR128RegClass.contains(Reg))
----------------
craig.topper wrote:
> Can we use TargetRegisterInfo::getRegEncoding() to simplify this?
I can simplify the code by defining a lambda function called isAVX512RegOnly(unsigned Reg) and use it to check on the Reg operand to be between X86::ZMM0 and X86::ZMM31 or between X86::YMM16 and X86::YMM31 or between X86:XMM16 and X86::XMM31 as follows:

auto isAVX512RegOnly = [](unsigned Reg) {
    if (Reg >= X86::ZMM0 && Reg <= X86::ZMM31)
        return true;

    if (Reg >= X86::XMM16 && Reg <= X86::XMM31)
        return true;

    if (Reg >= X86::YMM16 && Reg <= X86::YMM31)
        return true;

    return false;
 };





Repository:
  rL LLVM

https://reviews.llvm.org/D27901