[PATCH] D17095: [X86] Add X86FixupSeparateStack pass

[Michael LeMay via llvm-commits]

mlemay-intel added a comment.

Sorry for my delayed response.  Activity notifications have not been reaching my inbox.


================
Comment at: lib/Target/X86/X86.td:246
@@ -242,2 +245,3 @@
+                       " by adding segment override prefixes (X86-32 only).">;
 
 //===----------------------------------------------------------------------===//
----------------
craig.topper wrote:
> Can this be done with a command line switch in X86TargetMachine.cpp like VZeroUpper? I don't see that this needs to be a feature bit.
Your suggestion should work fine at the LLVM level, but then how would you suggest passing the flag to Clang?

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

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

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:599
@@ +598,3 @@
+
+    bool HasEVEX_K = TSFlags & X86II::EVEX_K;
+    bool HasVEX_4V = TSFlags & X86II::VEX_4V;
----------------
craig.topper wrote:
> delena wrote:
> > Why masked instructions are out of scope?
> > DestMem can't be VEX_4V.
> DestMem can be VEX_4V. We check for it in both MCCodeEmitter and the Disassembler. I believe at least vmaskmovpd uses it.
I assume that VEX-encoded instructions are not used for storing the stack pointer.  I think that handling them would increase the complexity of line 610 for computing the operand number of the base register (I looked at X86MCCodeEmitter::EmitVEXOpcodePrefix to understand how these instructions are represented in LLVM).  Thus, I skip them to avoid that extra complexity.  I'll add a comment to note this decision.

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:610
@@ +609,3 @@
+      unsigned StackPtrStoreBaseReg =
+        I->getOperand(MemoryOperand + X86::AddrBaseReg).getReg();
+
----------------
delena wrote:
> Add assert (MemoryOperand != -1)
X86II::getMemoryOperandNo always returns 0 for instructions of this form.  I'll add a comment noting this assumption.

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

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:661
@@ +660,3 @@
+      AddrRegReqs *SuccReqs = new AddrRegReqs(Reqs);
+      Changed |= processBasicBlock(Succ, SuccReqs);
+    } else
----------------
delena wrote:
> Do you need to delete SuccReqs?
Ownership of the pointer is assigned to a shared_ptr in processBasicBlock.  However, I will revise processBasicBlock to accept a shared_ptr parameter so that the pointer ownership is more clear.

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:677
@@ +676,3 @@
+
+  assert(!STI->is64Bit() && "Only X86-32 is supported.");
+
----------------
delena wrote:
> Do you check this when you add the pass?
I think it may make sense to keep this check here so that an error message is emitted if the associated command line option is specified for an unsupported target.

================
Comment at: lib/Target/X86/X86FixupSeparateStack.cpp:681
@@ +680,3 @@
+
+  return processBasicBlock(&MF.front(), new AddrRegReqs(TRI));
+}
----------------
delena wrote:
> Who deletes the object created by "new" ?
This pointer is assigned to a shared_ptr in processBasicBlock.


http://reviews.llvm.org/D17095