[PATCH] D17095: [X86] Add X86FixupSeparateStack pass

[Elena Demikhovsky via llvm-commits]

delena added a subscriber: delena.

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:593
@@ +592,3 @@
+    int MemoryOperand = -1;
+    if (I->mayLoadOrStore()) {
+      // Determine where the memory operand starts, if present.
----------------
Are you working with load? I see PUSH32r and MRMDestMem - DestMem looks like store

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:596
@@ +595,3 @@
+      MemoryOperand = X86II::getMemoryOperandNo(TSFlags, Opcode);
+      if (MemoryOperand != -1) MemoryOperand += X86II::getOperandBias(Desc);
+    }
----------------
New line after )

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:599
@@ +598,3 @@
+
+    bool HasEVEX_K = TSFlags & X86II::EVEX_K;
+    bool HasVEX_4V = TSFlags & X86II::VEX_4V;
----------------
Why masked instructions are out of scope?
DestMem can't be VEX_4V.

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:610
@@ +609,3 @@
+      unsigned StackPtrStoreBaseReg =
+        I->getOperand(MemoryOperand + X86::AddrBaseReg).getReg();
+
----------------
Add assert (MemoryOperand != -1)

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:638
@@ +637,3 @@
+    for (MachineOperand &Op : I->operands()) {
+      if (!Op.isReg())
+        continue;
----------------
if (!Op.isReg() || !Op.isDef())
  continue;

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:661
@@ +660,3 @@
+      AddrRegReqs *SuccReqs = new AddrRegReqs(Reqs);
+      Changed |= processBasicBlock(Succ, SuccReqs);
+    } else
----------------
Do you need to delete SuccReqs?

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:677
@@ +676,3 @@
+
+  assert(!STI->is64Bit() && "Only X86-32 is supported.");
+
----------------
Do you check this when you add the pass?

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:681
@@ +680,3 @@
+
+  return processBasicBlock(&MF.front(), new AddrRegReqs(TRI));
+}
----------------
Who deletes the object created by "new" ?

================
Comment at: lib/Target/X86/X86TargetMachine.cpp:281
@@ -280,1 +280,3 @@
+
+  addPass(createX86FixupSeparateStack());
 }
----------------
for 32-bit only?


http://reviews.llvm.org/D17095