This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shiva0217 on May 8 2018, 11:45 PM.

Details

Reviewers
asb
apazos
Summary

.align N alignment fixed by assembler may be changed when the linker relaxation enabled.
In this case, we have to insert N-2 bytes NOPs and R_RISCV_ALIGN relocation type.
In this way, Linker could detect R_RISCV_ALIGN and fix the alignment by removing
NOPs.

To do this:

  1. Add parseDirectiveAlign in RISCVAsmParser to parse .align and .p2align directive and then insert PseudoNOPs instruction.
  1. Add expandNOPs in RISCVMCCodeEmitter to expand PseudoNOPs to NOPs with R_RISCV_ALIGN relocation type.
  1. Add disableAlignmentPaddingInTextSectiontarget hook in MCAsmBackend to disable generic padding for .align and let PseudoNOPs deal with it.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.May 8 2018, 11:45 PM

Hi Shiva,

Can you please rebase your patch. It is out of date

Rebase the patch to trunk@332828

shiva0217 updated this revision to Diff 147768.May 21 2018, 5:04 AM
shiva0217 edited the summary of this revision. (Show Details)

Rename thd target hook to disableAlignmentPaddingInTextSection.

asb added a comment.May 23 2018, 7:25 AM

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?

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1034

What about .balign?

1041

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.

In D46630#1109492, @asb wrote:

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?

Hi Alex. My first thought is to implement by MCAlignFragment, too. But R_RISCV_ALIGN has to insert to the first NOP. So that linker could know where are the Nop sequences start.
I can't find a way to insert R_RISCV_ALIGN by MCAlignFragment infrastructure. So I insert PseudoNOP which could indicate the first NOP position. Not sure it's the best way to do so.

asb added a comment.May 24 2018, 12:58 AM

I think that customising the handling of MCAlignFragment is going to be the more correct (with regards to the LLVM MC design and layering) and maintainable way.

I haven't fully stepped through the ordering of function calls when producing an object. Is your concern that the current call to writeNopData from writeFragment comes too late to create the relocation?

In D46630#1110712, @asb wrote:

I think that customising the handling of MCAlignFragment is going to be the more correct (with regards to the LLVM MC design and layering) and maintainable way.

I haven't fully stepped through the ordering of function calls when producing an object. Is your concern that the current call to writeNopData from writeFragment comes too late to create the relocation?

  SMLoc FirstNOPLoc = NOP.getLoc();
  Fixups.push_back(MCFixup::create(0, TotalBytes, MCFixupKind(RISCV::fixup_riscv_align), FirstNOPLoc));

When creating fixup_riscv_align, we need to indicate the first nop position which is recorded by SMLoc data structure.
I can't find a way to implement it in writeFragment yet.

Hi Alex. I find a way to emit fixup_riscv_align without introducing PseudoNOPs as you suggest.
The revision D47755 introducing the new target hooks to calculate the nops we need to insert and insert fixup_riscv_align for alignment directive.
Could you help me to check the new approach is ok or not?
Thanks.

shiva0217 abandoned this revision.Jun 25 2018, 1:24 AM

There is a new revision D47755 to replace this one.