Page MenuHomePhabricator

[XRay] Support AArch64 in LLVM
ClosedPublic

Authored by rSerge on Nov 8 2016, 12:26 PM.

Details

Summary

This patch adds XRay support in LLVM for AArch64 targets.
This patch is one of a series:

Diff Detail

Repository
rL LLVM

Event Timeline

rSerge updated this revision to Diff 77234.Nov 8 2016, 12:26 PM
rSerge retitled this revision from to [XRay] Support AArch64 in LLVM.
rSerge updated this object.
rSerge added reviewers: rengolin, dberris.
rSerge added subscribers: iid_iunknown, llvm-commits.
rSerge updated this object.Nov 8 2016, 12:28 PM
rSerge updated this object.Nov 8 2016, 12:32 PM
dberris requested changes to this revision.Nov 8 2016, 3:14 PM
dberris edited edge metadata.
dberris added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
159 ↗(On Diff #77234)

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.

210–212 ↗(On Diff #77234)

Probably remove '{}' for single line bodies?

lib/Target/AArch64/AArch64Subtarget.h
176 ↗(On Diff #77234)

I don't think you need the virtual here especially since you mark it override anyway.

This revision now requires changes to proceed.Nov 8 2016, 3:14 PM
rSerge added inline comments.Nov 9 2016, 2:04 PM
lib/Target/AArch64/AArch64AsmPrinter.cpp
159 ↗(On Diff #77234)

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();
}
210–212 ↗(On Diff #77234)

Changing.

lib/Target/AArch64/AArch64Subtarget.h
176 ↗(On Diff #77234)

Changing.

rSerge updated this revision to Diff 77394.Nov 9 2016, 2:14 PM
rSerge edited edge metadata.
rSerge marked 2 inline comments as done.
dberris added inline comments.Nov 9 2016, 3:02 PM
lib/Target/AArch64/AArch64AsmPrinter.cpp
159 ↗(On Diff #77234)

Yep, I think you found it. Would be happy to wait for the refactoring to make this simpler moving forward when we cover more architectures.

rSerge updated this revision to Diff 77538.Nov 10 2016, 12:47 PM
rSerge edited edge metadata.
rSerge updated this revision to Diff 77541.Nov 10 2016, 12:52 PM
amehsan added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
155 ↗(On Diff #77541)

extra space in the beginning can be removed.

rSerge marked an inline comment as done.Nov 10 2016, 12:56 PM
rSerge added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
159 ↗(On Diff #77234)

My team has suggested to defer the refactorring to a separate patch, so to keep the current patch AArch64-only. That refactoring would affect multiple CPUs, but the changes would have in common that they are all for ELF.
So I've uploaded a new version with code duplication and a TODO for the refactoring.

dberris accepted this revision.Nov 10 2016, 3:55 PM
dberris edited edge metadata.

Just a couple style issues, thanks @rSerge -- I'll look forward to the refactoring afterwards to consolidate some of the ELF-specific XRay instrumentation stuff potentially at a higher level at a later time.

lib/Target/AArch64/AArch64AsmPrinter.cpp
162–170 ↗(On Diff #77541)

Since these are single-statement bodies for the true and false branches, LLVM style seems to prefer not having curlies around them:

if (...)
  Section = ...;
else
  Section = ...;

Also, if you do need compound statements in the branches, the else should be on the same line as the previous }. So:

if (...) {
  ...
} else {
  ...
}
685–686 ↗(On Diff #77541)

To keep it consistent with surrounding style, maybe add an empty line between the return; and the case ...:? i.e.

case TargetOpcode::...:
  ...
  return;

case TargetOpcode::...;
  ...
  return;
This revision is now accepted and ready to land.Nov 10 2016, 3:55 PM
rSerge updated this revision to Diff 77672.Nov 11 2016, 2:32 PM
rSerge edited edge metadata.
rSerge marked 5 inline comments as done.Nov 11 2016, 2:44 PM
rSerge added inline comments.
lib/Target/AArch64/AArch64AsmPrinter.cpp
155 ↗(On Diff #77541)

Leaving 1 space after //

162–170 ↗(On Diff #77541)

Thanks for explaining. It's copy-pasted from your code =)
Changing here...

685–686 ↗(On Diff #77541)

Changing.

rSerge updated this revision to Diff 78217.Nov 16 2016, 10:32 AM
rSerge marked 2 inline comments as done.

Rebased to the latest version of LLVM sources.

This revision was automatically updated to reflect the committed changes.

Now that this has been landed (just waiting for the build bots to finish going through the tests) it would be great if we also had tests for aarch64 and maybe arm7 specifically to look for the assembler patterns we want similar to the x86_64 tests.

rengolin edited edge metadata.Nov 17 2016, 1:43 AM

Now that this has been landed (just waiting for the build bots to finish going through the tests) it would be great if we also had tests for aarch64 and maybe arm7 specifically to look for the assembler patterns we want similar to the x86_64 tests.

Yes, please! :)

Oops, I remember that I implemented a test for this, but got lost it somewhere... looking...

No, it was a test for tail calls.
Here is a test and a fix for the jump instruction emitted in sleds: https://reviews.llvm.org/D26805