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

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1328
+    //   br   x16
+    setSystemFlag(Insts[0], getIntArgRegister(1));
+    createPopRegisters(Insts[1], AArch64::X0, AArch64::X1);
----------------
getIntArgRegister(1) should be replaced with AArch64::X1


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1345
+  void convertIndirectCallToLoad(MCInst &Inst, MCPhysReg Reg) override {
+    if (Inst.getOpcode() == AArch64::BL || Inst.getOpcode() == AArch64::BLR ||
+        Inst.getOpcode() == AArch64::BR || Inst.getOpcode() == AArch64::B) {
----------------
Don't we need to remove the tailcall annotation like in the X86 code?


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1370
+    InstructionListType Insts(4);
+    int shift = 48;
+    for (int i = 0; i < 4; i++, shift -= 16) {
----------------
Shift


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1371
+    int shift = 48;
+    for (int i = 0; i < 4; i++, shift -= 16) {
+      Insts[i].setOpcode(AArch64::MOVKXi);
----------------
I


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1424-1428
+    for (int I = MCPlus::getNumPrimeOperands(CallInst),
+             E = CallInst.getNumOperands();
+         I != E; ++I) {
+      Insts.back().addOperand(CallInst.getOperand(I));
+    }
----------------
This is not doing what the comment says -- try the same sequence used in X86:

  stripAnnotations(Insts.back())
  moveAnnotations(std::move(CallInst), Insts.back());




================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:1448
+    //   b       IndCallHandler
+    const MCPhysReg TempReg = getIntArgRegister(0);
+    InstructionListType Insts;
----------------
same - delete TempReg and use X0 directly like you do on line 1451


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

https://reviews.llvm.org/D151899