[PATCH] D145687: [BOLT] Add minimal RISC-V 64-bit support

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Awesome!



================
Comment at: bolt/lib/Core/BinaryContext.cpp:750-752
+MCSymbol *BinaryContext::getOrCreateAbsoluteSymbol(StringRef Name) {
+  return Ctx->getOrCreateSymbol(Name);
+}
----------------
Why are we creating a new method to do the same thing as the above one?


================
Comment at: bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp:296
+
+  void fixCalls(BinaryFunction &BF) override {
+    for (auto &BB : BF) {
----------------
BOLT's libTarget is designed in a way such that it doesn't know about BOLT's core data structures other than MCInst and the MCPlusBuilder class (see the other backend implementations). libBOLTCore depends on libBOLTTarget, but not vice-versa (except for the MCPlusBuilder base class itself, which is in BOLTCore). 

So the way forward here would be to implement this loop over all instructions in the pass itself, and only call the target backend for specific target-specific actions on MCInsts. For example, only the body of the loop here would be in "fixCalls".

Another thing we could do is to rename fixCalls to fixRISCVCall, so it is obvious to other backends that this function is exclusive of RISCV and that they don't have to implement it.


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

https://reviews.llvm.org/D145687