[PATCH] D19046: Introduce a "patchable-prologue" function attribute

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

(Just about to update the description).


================
Comment at: lib/Target/X86/X86MCInstLower.cpp:837-838
@@ -836,1 +836,4 @@
+    (void) OnlyOneNop;
+    assert((!OnlyOneNop || NumBytes == 0) &&
+           "Allowed only one nop instruction!");
   } // while (NumBytes)
----------------
dberris wrote:
> I'm not sure this assertion makes sense here. I would have thought this assert should have been done in the calling code, that it doesn't ask for a single nop in the first place?
That's the first point under the "There is some cleanup we can do after this" note I sent in with this update.  :)  Basically, I'd rather not make this already large patch any larger if it can be helped.  NFC cleanups like these are easy to do once the hard stuff has been reviewed and checked in, IMO.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:948-949
@@ +947,4 @@
+    } else {
+      EmitNops(*OutStreamer, MinSize, Subtarget->is64Bit(), getSubtargetInfo(),
+               /* OnlyOneNop = */ true);
+    }
----------------
dberris wrote:
> So in this branch, MinSize != 2 or Opcode != X86::PUSH64r.
> 
> Question: If you check instead whether the function where MI is included in had the correct type of patchable-function attribute, and see that the nops being added is less than 2, you can assert and say this is actually a bug in a higher implementation detail? i.e. this would be a bug in the insertion of this instruction.
> 
> This way you wouldn't need to touch EmitNops.
> Question: If you check instead whether the function where MI is included in had the correct type of patchable-function attribute, and see that the nops being added is less than 2, you can assert and say this is actually a bug in a higher implementation detail? i.e. this would be a bug in the insertion of this instruction.

Do you mean something like:

```
assert(MinSize < 2 && !MF.getPatchableFnType() == "prologue-short-redirect");
```

I'd say that is an incorrect layering.  It feels cleaner for MC to not have to know why the `PATCHABLE_OP` of a certain variety is present.  It should just understand its end of the contract of how it needs to lower `PATCHABLE_OP`.


http://reviews.llvm.org/D19046