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

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 added a comment.

Thank you for this patch :)



================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:1461
+  // Skip the first special entry since no relocation points to it.
+  uint64_t InstrOffset = 32;
+
----------------
Is it first PLT entry skipped? I don't think it is good idea to set constant size here in that case. Maybe it is possible to make more universal like it is done in aarch64? there are no PLT entry size assumptions too as I remember


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2096
+    // TODO Verification is made difficult by the many HI/LO relocation pairs.
+    if (BC->isRISCV())
+      return true;
----------------
Please add || for aarch64 above


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2624
   if (!ReferencedSection && !ForceRelocation) {
+    // Check for ABS symbols. Used e.g., for __global_pointer$ on RISC-V
+    auto RelSymbol = Rel.getSymbol();
----------------
We would need to check this code. Please provide more info about example of such symbols, sections it is contained/referenced in and segments.
Is there any chance that the problem might be solved by this commit https://github.com/llvm/llvm-project/commit/0776fc32b15dc76c6b43c41fc4471552833265de ? 


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3255
     Function.postProcessCFG();
+    BC->MIB->postProcessFunction(Function);
 
----------------
Probably better to move it to arch-specific pass like it is currently done in ADRRelaxationPass or VeneerElimination for example


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

https://reviews.llvm.org/D145687