[PATCH] D19904: XRay: Add entry and exit sleds

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Some minor comments inline.  I'm not familiar with all of the intricacies of the backend, so I'll CC some other people who are likely to have more perspective on this.


================
Comment at: include/llvm/Target/Target.td:957
@@ +956,3 @@
+}
+def PATCHABLE_TAIL_CALL : Instruction {
+  let OutOperandList = (outs);
----------------
Looks like this isn't used yet?  If so, I'd suggest dropping it completely for now.

================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:80
@@ +79,3 @@
+          if (MO.isReg())
+            MIB.addReg(MO.getReg(), RegState::Implicit);
+        }
----------------
Why do you care only about register operands here?

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1068
@@ +1067,3 @@
+  auto Attr = Fn->getFnAttribute("function-instrument");
+  bool AlwaysInstrument = false;
+  if (!Attr.hasAttribute(Attribute::None) && Attr.isStringAttribute() &&
----------------
Why not

```
AlwaysInstrument = !Attr.hasAttribute(Attribute::None) && Attr.isStringAttribute() &&
      Attr.getValueAsString() == "xray-always";
```

?

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1069
@@ +1068,3 @@
+  bool AlwaysInstrument = false;
+  if (!Attr.hasAttribute(Attribute::None) && Attr.isStringAttribute() &&
+      Attr.getValueAsString() == "xray-always")
----------------
I don't quite grok the Attributes API, but I think the `!Attr.hasAttribute(Attribute::None)` bit is redundant -- if `isStringAttribute` return `true`, then `Attr.hasAttribute(Attribute::None)` will be false.

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1072
@@ +1071,3 @@
+    AlwaysInstrument = true;
+  Sleds.push_back(
+      {Sled, CurrentFnSym, SledKind::FUNCTION_ENTER, AlwaysInstrument});
----------------
emplace_back ?

================
Comment at: lib/Target/X86/X86MCInstLower.cpp:1100
@@ +1099,3 @@
+void X86AsmPrinter::LowerPATCHABLE_RET(const MachineInstr &MI,
+                                            X86MCInstLower &MCIL) {
+  // Since PATCHABLE_RET takes the opcode of the return statement as an
----------------
Indent is off.


Repository:
  rL LLVM

http://reviews.llvm.org/D19904