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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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
----------------
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.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:54
+    void AddTableEntry(EvexToVexTableType &EvexToVexTable,           
+                          uint16_t EvexOp, uint16_t VexOp);
+  
----------------
The second line is indented too far.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:125
+void EvexToVexInstPass::AddTableEntry(EvexToVexTableType &EvexToVexTable,
+                            uint16_t EvexOp, uint16_t VexOp) {
+  EvexToVexTable[EvexOp] = VexOp;
----------------
This line should be indented further to line up arguments.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:151
+    // check for EVEX instructions only
+    if (!(Desc.TSFlags & X86II::EVEX))
+      continue;
----------------
This should be (Desc.TSFlags & X86II::EncodingMask) == X86II::EVEX)

X86II::EVEX is not a bit mask its a value for the encoding field.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:159
+    // EVEX_V512 instr: bit EVEX_L2 = 1; bit VEX_L = 0
+    bool isEVEX_V512 = ((Desc.TSFlags & X86II::EVEX_L2) && 
+                        !(Desc.TSFlags & X86II::VEX_L));
----------------
Variable names should start with a capital letter.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:162
+
+    //check for non EVEX_V512 instrs only:
+    if (isEVEX_V512)
----------------
Comments in LLVM are generally written as sentences starting with a capitalized first word and ending with a period.


================
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) && 
----------------
What do we get out of having two different DenseMaps? Couldn't we have 128 and 256 in the same map?


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:206
+      
+      if (X86::VR256RegClass.contains(Reg) ||
+          X86::VR128RegClass.contains(Reg))
----------------
Can we use TargetRegisterInfo::getRegEncoding() to simplify this?


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:224
+    const MCInstrDesc &MCID = TII->get(NewOpc);
+    MI->setDesc (MCID); 
+    MI->setAsmPrinterFlag((MachineInstr::CommentFlag)AC_EXEX_2_VEX);
----------------
Remove space before parentheses


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1294
 
+  // Add a comment about EVE-2-VEX compression for AVX-512 instrs that
+  // are compressed from EVEX encoding to VEX encoding.
----------------
EVE should be EVEX


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:403
     addPass(createX86FixupLEAs());
-  }
+	addPass(createX86EvexToVexInsts());
+  }  
----------------
Fix the indentation here.


Repository:
  rL LLVM

https://reviews.llvm.org/D27901