[PATCH] D19908: [X86] Support the "ms-hotpatch" attribute.

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: lib/CodeGen/PatchableFunction.cpp:43
@@ -42,3 +42,3 @@
   if (!MF.getFunction()->hasFnAttribute("patchable-function"))
     return false;
 
----------------
sanjoy wrote:
> I presume you're handling the x86-64 case here?  If so, how about handling both the 32bit and 64bit case here?  On 32 bits, you can just emit a `mov %edi, %edi` instead of a `PATCHABLE_OP`?  That way related bits stay together.
+1

================
Comment at: lib/CodeGen/PatchableFunction.cpp:49
@@ -49,1 +48,3 @@
+  assert(PatchType == "prologue-short-redirect" ||
+         PatchType == "ms-hotpatch" && "Only possibilities today!");
 #endif
----------------
GCC will give you a -Wparentheses warning on this.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2504-2507
@@ -2505,1 +2503,6 @@
+       Subtarget.isTargetCygMing() &&
+       Fn->getName() == "main") ||
+      (Fn->hasFnAttribute("patchable-function") &&
+       Fn->getFnAttribute("patchable-function").getValueAsString() ==
+           "ms-hotpatch"))
     FuncInfo->setForceFramePointer(true);
----------------
Why do we need this change? The patchable nop comes before the prologue. Surely this isn't use for some kind of on-stack-replacement.

================
Comment at: test/CodeGen/X86/ms-hotpatch-attr.ll:4
@@ +3,3 @@
+
+; CHECK-32: .space 64,204
+; CHECK-32-LABEL: foo:
----------------
Can you add a check for "align"? Otherwise there's no difference between the constant pool entry and the function label override.


http://reviews.llvm.org/D19908