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

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 abandoned this revision.
shiva0217 added inline comments.


================
Comment at: test/MC/RISCV/relocations.s:93
 
 c.jal foo
+# RELOC: R_RISCV_RVC_JUMP
----------------
sabuasal wrote:
> sabuasal wrote:
> > 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. 
> > 
> > 
> > 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.
> 
> 
> 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D44971