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

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: lib/MC/MCAsmStreamer.cpp:303
 /// verbose assembly output is enabled.
-void MCAsmStreamer::AddComment(const Twine &T) {
+void MCAsmStreamer::AddComment(const Twine &T, bool EOL) {
   if (!IsVerboseAsm) return;
----------------
Have you considered using GetCommentOS() or EmitRawComment() before modifying this method?


================
Comment at: lib/MC/MCAsmStreamer.cpp:308
+
+  // By deafult each comment goes on its own line.
+  if (EOL)
----------------
The 'by default' comment should be placed where EOL is initialized by default to true?


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:1
+//===----------------------- X86EvexToVex.cpp ----------------------------===//
+// Compress EVEX instructions to VEX encoding when possible to reduce code size
----------------
Can you please run clang-format on this file? it will fix some of Craig's comments about indentation and perhaps a few more glitches.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:105
+  TII = MF.getSubtarget<X86Subtarget>().getInstrInfo();
+  MachineBasicBlock &FirstMBB = MF.front();
+  bool rc = false;
----------------
What if MF is empty?


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:108
+
+  DEBUG(dbgs() << "Start X86EvexToVexInsts\n";);
+  
----------------
Please remove this DEBUG() macro and the one in line 118 as they don't contribute much.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:112
+  /// EVEX encoded instrs by VEX encoding when possible:
+  for (MachineFunction::iterator I = FirstMBB.getIterator(), 
+       E = FirstMBB.getParent()->end(); I != E; ++I) {
----------------
Please consider:
for (MachineBasicBlock &MBB : F)
  rc = ....


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:130
+
+// For EVEX instructions that can be encoded using VEX encodig, 
+// replace them by the VEX encoding in order to reduce size:
----------------
*encoding


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:142
+  
+  bool instrWasUpdated = false;
+
----------------
There is a convention in other passes to name this variable 'Changed'


================
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));
----------------
craig.topper wrote:
> Variable names should start with a capital letter.
Consider dropping this variable since it's used only in the following statement.


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:198
+   
+    for (unsigned i = 0, e = I->getNumOperands(); i != e; ++i) {
+      const MachineOperand &MO = I->getOperand(i);   
----------------
consider this:
for (const MachineOperand &MO : I->operands()) {


================
Comment at: lib/Target/X86/X86EvexToVex.cpp:222
+
+    MachineInstr *MI = &*I;
+    const MCInstrDesc &MCID = TII->get(NewOpc);
----------------
if you change the loop header in line 145 to the following you won't need this extra variable:
for (MachineInstr &MI : MBB) {


================
Comment at: test/CodeGen/X86/evex-to-vex-compress.mir:17
+  # CHECK: bb.0:
+  # CHECK: VMOVAPDYmr                    %rdi, 1, _, 0, _, %ymm0
+  # CHECK: %ymm0 = VMOVAPDYrm            %rip, 1, _, %rax, _
----------------
It's hard to follow this enormous test. Can you please put the '# CHECK:' line above and adjacent to its corresponding input instruction?


Repository:
  rL LLVM

https://reviews.llvm.org/D27901