[PATCH] D47755: [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for alignment when linker relaxation enabled

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 marked 2 inline comments as done.
shiva0217 added inline comments.


================
Comment at: lib/MC/MCAssembler.cpp:840
+      } else if (dyn_cast<MCAlignFragment>(&Frag)) {
+        MCAlignFragment &AF = cast<MCAlignFragment>(Frag);
+        // Insert fixup type for the alignment if the target define
----------------
jrtc27 wrote:
> This should be moved into the if (and the `dyn_cast` should never have existed on its own in the first place; if the result is discarded it should be an `isa`).
Hi James,
Thanks for the correction.


================
Comment at: lib/MC/MCAssembler.cpp:844
+        if (auto *ELFSec = dyn_cast<const MCSectionELF>(AF.getParent())) {
+          if (ELFSec->getSectionName() == StringRef(".text"))
+            getBackend().insertFixupForAlignDirectiveInTextSection(*this,
----------------
jrtc27 wrote:
> Hard-coding the section name seems wrong. What if somebody uses a section attribute? GNU as will do the right thing for any executable section regardless of its name, and we should too; from an initial glance it seems that `MCSection::UseCodeAlign()` will tell you exactly the right thing, and as a bonus is no longer ELF-specific.
It seems to be a much graceful and universal approach, thanks a lot.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D47755