Page MenuHomePhabricator

[RISCV] Override fixupNeedsRelaxationAdvanced to avoid MC relaxation always promote to 32-bit form when -mrelax enabled
AbandonedPublic

Authored by shiva0217 on Mar 27 2018, 10:38 PM.

Details

Summary

Override fixupNeedsRelaxationAdvanced to avoid RISCV MC Branch Relaxation always promote 16-bit branches to 32-bit form when RISCV linker relaxation enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Mar 27 2018, 10:38 PM
sabuasal added inline comments.Mar 28 2018, 11:34 AM
test/MC/RISCV/relocations.s
93

Hi Shiva,

I noticed that gcc-as will relax jumps to unresolved symbols. only generates rvc-jump for resolved ones.

echo "c.jal foo" | riscv32-unknown-linux-gnu-as -march=rv32imc -o ./a.out && riscv32-unknown-linux-gnu-objdump -M no-aliases -dr ./a.out
00000000 <.text>:

0:   000000ef                jal     ra,0x0
                    0: R_RISCV_JAL  foo

echo "foo: nop; jal foo" | riscv32-unknown-linux-gnu-as -march rv32imc -o ./a.out && riscv32-unknown-linux-gnu-> objdump -d -r -M no-aliases ./a.out

0: 0001 c.addi zero,0

2:   3ffd                    c.jal   0 <foo>
                     2: R_RISCV_RVC_JUMP     foo

We can still maintain this behavior by checking the "Fixup" kind and "resolved" in fixupNeedsRelaxationAdvanced. However, this is a better code size optimization I think if the c.jal target appears to be within reach during linking.
@asb
What do you think?

sabuasal edited reviewers, added: sabuasal; removed: sameer.abuasal.Mar 28 2018, 11:35 AM
zzheng added a subscriber: zzheng.Apr 3 2018, 12:05 PM
asb added inline comments.Apr 5 2018, 3:51 AM
test/MC/RISCV/relocations.s
93

It would be great to expand the tests to cover the case of an unresolved jump target. Matching the gcc behaviour seems a sensible starting point.

shiva0217 added inline comments.Apr 11 2018, 6:24 PM
test/MC/RISCV/relocations.s
93

Hi @sabuasal. Resolved will always false when -mrelax enabled due to shouldForceRelocation. If we relax c.jal by checking "Fixup" and "Resolved", c.jal will always relax to jal when -mrelax enabled even if the symbol actually could be resolved.
To match the gcc behaviour as @asb's comments, we have to identify the symbol is actually could be resolved to avoid always relax to expansion form. Perhaps we still need https://reviews.llvm.org/D44887? Any thoughts?

sabuasal added inline comments.Apr 12 2018, 9:15 AM
test/MC/RISCV/relocations.s
93

Hi Shiva,

if (STI.getFeatureBits()[RISCV::FeatureRelax]) is false we know that Resolved is false because the symbol is actually unresolved, not that it was set to false due to shouldForceRelection() behaviour. If that applies and the FixUp kind you get belongs to a compression fixup then we should just relax the instruction. This way we can match gcc for compressed jumps to unresolved symbols.

What do you think?

93

If after that you think the code will be too ugly I an fine going back to your original patch.

shiva0217 added inline comments.Apr 13 2018, 12:26 AM
test/MC/RISCV/relocations.s
93

Hi @sabuasal,

bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
                                  uint64_t Value,
                                  const MCRelaxableFragment *DF,
                                  const MCAsmLayout &Layout) const override {
  if (!STI.getFeatureBits()[RISCV::FeatureRelax] && !Resolved)
    return true;
  return fixupNeedsRelaxation(Fixup, Value, DF, Layout);
}

Yes, we could match gcc behaviour for unresolved symbols when relaxation disabled, but c.jal still not relax to jal for unresolved symbols when relaxation enabled.
I think gcc always relax c.jal to jal for unresolved symbols because if the symbols are outside of the compile unit, it's very likely that the symbol offset couldn't fit into c.jal operand field. I'll try to push https://reviews.llvm.org/D44887 if you think it's reasonable, too.

sabuasal added inline comments.Apr 13 2018, 9:32 AM
test/MC/RISCV/relocations.s
93
bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
                                  uint64_t Value,
                                  const MCRelaxableFragment *DF,
                                  const MCAsmLayout &Layout) const override {
  if (!STI.getFeatureBits()[RISCV::FeatureRelax] && !Resolved)
    return true;

  if (STI.getFeatureBits()[RISCV::FeatureRelax])
     return true;

  return fixupNeedsRelaxation(Fixup, Value, DF, Layout);
}
sabuasal added inline comments.Apr 13 2018, 10:10 AM
test/MC/RISCV/relocations.s
93

Oops, please ignore the above comment I hit submit without realizing it.

sabuasal added inline comments.Apr 15 2018, 9:51 PM
test/MC/RISCV/relocations.s
93

I think gcc always relax c.jal to jal for unresolved symbols because if the symbols are outside of the compile unit, it's very likely that the symbol offset couldn't fit into c.jal operand field.

Yea I believe that is true and it make sense to me.

Yes, we could match gcc behavior for unresolved symbols when relaxation disabled, but c.jal still not relax to jal for unresolved symbols when relaxation enabled.

I just took a look at the assembler framework again. The problem is that the relaxation logic, which happens during the layoutOnce() shares some logic with the Relocation recording by calling evaluateFixup which checks shouldForceRelocation in the backend.

The only way I can see this working is if the fixupNeedsRelocationAdvnced() duplicates the work in evaluateFixup() to get the "Value" and "IsResolved". This way we can separate the relocation and relaxation logic but that does sound too invasive.

I think you are right, I might heave lead you in the wrong way, lets o back to the old patch.

shiva0217 abandoned this revision.Apr 15 2018, 11:02 PM
shiva0217 added inline comments.
test/MC/RISCV/relocations.s
93

Hi Sabuasal. Utilizing existing target hook always better than introducing a new one. Your comment's lead us to explore the possibility and make the new target hook more reasonable. Thanks for your comments and let's push the old one.