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

[Job Noorman via Phabricator via llvm-commits]

jobnoorman marked 3 inline comments as done.
jobnoorman added inline comments.


================
Comment at: bolt/lib/Core/BinaryContext.cpp:750-752
+MCSymbol *BinaryContext::getOrCreateAbsoluteSymbol(StringRef Name) {
+  return Ctx->getOrCreateSymbol(Name);
+}
----------------
rafauler wrote:
> Why are we creating a new method to do the same thing as the above one?
Nice catch! I think this method went through several rounds of refactoring and ended up the same as another one without me noticing :) Removed.


================
Comment at: bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp:296
+
+  void fixCalls(BinaryFunction &BF) override {
+    for (auto &BB : BF) {
----------------
rafauler wrote:
> 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.
Fair point, I gave it a shot: the pass now loops over all instructions, calls a new method MCPlusBuilder::isRISCVCall to identify AUIPC/JALR pairs and then fixes-up the JALR.

It still feels unfortunate to have target-specific methods in MCPlusBuilder though. I wonder of some of them (like this one) could be prevented by allowing passes to be implemented in the backend library. Not something to be addressed by this patch though.


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

https://reviews.llvm.org/D145687