[PATCH] D26412: [XRay] Support AArch64 in LLVM

[Serge Rogatch via llvm-commits]

rSerge added inline comments.


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:159
+      "xray_instr_map", ELF::SHT_PROGBITS,
+      ELF::SHF_ALLOC | ELF::SHF_GROUP | ELF::SHF_MERGE, 0,
+      CurrentFnSym->getName());
----------------
dberris wrote:
> I distinctly remember having to do something special for non-COMDAT sections, especially when doing section-per-function in x86. I suspect you'd want to replicate that logic here somehow, with a `//TODO` to merge the logic for ELF XRay sleds at a higher level for better re-use.
Yes, I see it now. Shall this code be CPU-independent? If so, let me look if I can refactor it in this patch...
The CPU-independent code seems to be

```
  if (Sleds.empty())
    return;
  if (Subtarget->isTargetELF()) {
    auto PrevSection = OutStreamer->getCurrentSectionOnly();
    auto Fn = MF->getFunction();
    MCSection *Section = nullptr;
    if (Fn->hasComdat()) {
      Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
                                         ELF::SHF_ALLOC | ELF::SHF_GROUP, 0,
                                         Fn->getComdat()->getName());
    } else {
      Section = OutContext.getELFSection("xray_instr_map", ELF::SHT_PROGBITS,
                                         ELF::SHF_ALLOC);
    }

    // Before we switch over, we force a reference to a label inside the
    // xray_instr_map section. Since EmitXRayTable() is always called just
    // before the function's end, we assume that this is happening after the
    // last return instruction.
    //
    // We then align the reference to 16 byte boundaries, which we determined
    // experimentally to be beneficial to avoid causing decoder stalls.
    MCSymbol *Tmp = OutContext.createTempSymbol("xray_synthetic_", true);
    OutStreamer->EmitCodeAlignment(16);
    OutStreamer->EmitSymbolValue(Tmp, 8, false);
    OutStreamer->SwitchSection(Section);
    OutStreamer->EmitLabel(Tmp);
    for (const auto &Sled : Sleds) {
         // CPU-dependent code block here (depends of 64- or 32- bit word width)
    }
    OutStreamer->SwitchSection(PrevSection);
  }
  Sleds.clear();
}
```


================
Comment at: lib/Target/AArch64/AArch64AsmPrinter.cpp:210-212
+  {
+    EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+  }
----------------
dberris wrote:
> Probably remove '{}' for single line bodies?
Changing.


================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:176
 
+  virtual bool isXRaySupported() const override { return true; }
+
----------------
dberris wrote:
> I don't think you need the virtual here especially since you mark it `override` anyway.
Changing.


https://reviews.llvm.org/D26412