[PATCH] D126177: [BOLT] [AArch64] Handle data markers spanning multiple functions

[Rafael Auler via Phabricator via llvm-commits]

rafauler accepted this revision.
rafauler added a comment.
This revision is now accepted and ready to land.

Thanks @treapster. I appreciate the robustness of YAML for this test here, but I do like to have the source code so it's easier to read. Do you think you could put the original assembly code in the test just as a reference, but don't assemble it, just add a comment that the YAML was generated out of that code?

I agree you can move the check to callers of getMarkerType. But if the intent in doing so is to move getMarkerType to SymbolRef, then I think it's best to do not go in that direction. SymbolRef is defined by another library, LLVM's Object lib, which is not BOLT-specific. I have two concerns regarding that, (1) I'm skeptical this would be useful outside of BOLT, (2) talking specifically about the SymbolRef class (ObjectFile.h), this is a very general class that is limited to only the very basic operations you can do on a symbol, and it is not ELF specific -- it's actually the general interface for all symbols in all object formats in LLVM. In contrast, getMarkerType is ELF-specific, AArch64 ABI-specific method to identify data. Because of that I think it would be best if we leave this in BinaryContext, which is BOLT's IR module class, containing general information we need to disassemble and process a binary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126177