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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added inline comments.


================
Comment at: test/MC/RISCV/relocations.s:93
 
 c.jal foo
+# RELOC: R_RISCV_RVC_JUMP
----------------
sabuasal wrote:
> shiva0217 wrote:
> > sabuasal wrote:
> > > sabuasal wrote:
> > > > shiva0217 wrote:
> > > > > asb wrote:
> > > > > > sabuasal wrote:
> > > > > > > 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?
> > > > > > > 
> > > > > > > 
> > > > > > 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.
> > > > > 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?
> > > > 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?  
> > > If after that you think the code will be too ugly I an fine going back to your original patch.
> > 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.
> ```
> 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);
> }
> ```
Oops, please ignore the above comment I hit submit without realizing it. 




Repository:
  rL LLVM

https://reviews.llvm.org/D44971