[PATCH] D46630: [RISCV] Insert NOPs and R_RISCV_ALIGN relocation type for .align directive when linker relaxation enabled

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Hi Shiva. As far as testing goes, I think we really need to see testing of corner cases around enabling/disabling rvc support in the same file.

Could you comment about the decision to introduce the PseudoNOP instruction? The obvious alternative approach would be to try to customise the code that handles MCAlignFragments, adding new hooks if necessary. Have you looked in any detail at doing it that way?



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1034
+  else if (enableLinkerRelax() &&
+           ((IDVal == ".align") || (IDVal == ".p2align")))
+    return parseDirectiveAlign(DirectiveID.getLoc());
----------------
What about .balign?


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1041
+bool RISCVAsmParser::parseDirectiveAlign(SMLoc L) {
+  StringRef AlignStr = getParser().parseStringToEndOfStatement().trim();
+  unsigned Align, Padding;
----------------
This ignores anything after the first argument to .align. Don't we need to duplicate some of the parsing logic from AsmParser::parseDirectiveAlign? I can certainly see that if I change one of the .p2 align to `.p2align 2, 1` then that directive is deleted entirely when testing asm printing with llvm-mc -filetype=asm.


Repository:
  rL LLVM

https://reviews.llvm.org/D46630