[PATCH] D151899: [BOLT] Instrumentation: Initial instrumentation support for AArch64

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:466-501
+  virtual void getSystemFlag(MCInst &Inst, MCPhysReg Reg) const {
+    llvm_unreachable("not implemented");
+  }
+
+  virtual void setSystemFlag(MCInst &Inst, MCPhysReg Reg) const {
+    llvm_unreachable("not implemented");
+  }
----------------
These 6 new member functions being added here are only locally used inside AArch64MCPlusBuilder.cpp, correct? If that's the case, remove them from the architecture-independent interface (MCPlusBuilder.h) and declare them as helper functions/local definitions inside AArch64MCPlusBuilder.cpp  outside of the class. I don't think they need to be member functions, so they can have visibility limited to the AArch64 file, e.g.:

  static void createPushRegisters(MCInst...
    Inst.clear();
    ...
  }

  InstructionListType createInstrumentedIndirectCall(MCInst &&CallInst,
                                                     MCSymbol *HandlerFuncAddr,
                                                     int CallSiteID,
                                                     MCContext *Ctx) override {
    ...
    createPushRegisters(...)


================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:1628-1638
+  virtual std::vector<MCInst> createIncMemory(MCPhysReg RegTo,
+                                              MCPhysReg RegTmp) const {
+    llvm_unreachable("not implemented");
+  }
+
+  virtual void atomicAdd(MCInst &Inst, MCPhysReg Reg1, MCPhysReg Reg2) const {
+    llvm_unreachable("not implemented");
----------------
Same for these functions. In general the idea is to avoid unnecessarily exporting functions to the generic architecture-independent interface because by doing so we are suggesting that other architectures should implement these functions, and we're further implying BOLT will use them somewhere in the architecture-independent code, when in reality we're just using them internally in our AArch64 implementation.


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1490
+    I += Addr.size();
+    storeReg(Instrs[I++], AArch64::X2, getSpRegister(CodePointerSize));
+    InstructionListType Insts = createIncMemory(AArch64::X0, AArch64::X2);
----------------
Do we support 32-bit pointers for AArch64? If not, I would just hardcode AArch64::SP here and in other locations an drop the CodePointerSize argument. If we do support, I think it would be nice to have tests that show we are working correctly for these binaries.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151899/new/

https://reviews.llvm.org/D151899