[PATCH] D153097: [RISCV] Make linker-relaxable instructions terminate MCDataFragment

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added a comment.

In D153097#4427239 <https://reviews.llvm.org/D153097#4427239>, @barannikov88 wrote:

> In D153097#4427091 <https://reviews.llvm.org/D153097#4427091>, @MaskRay wrote:
>
>> In D153097#4427010 <https://reviews.llvm.org/D153097#4427010>, @barannikov88 wrote:
>>
>>> `MCObjectStreamer::emitInstructionImpl` has some logic deciding whether the instruction can be appended to a data fragment or should go in its own fragment.
>>> I would expect the check to be there instead of `MCELFStreamer::emitInstToData`.
>>> For this to be possible `encodeInstruction` should be pulled out too, so that you can examine Fixups.
>>> As I can see, all implementations of `emitInstrToData` call `encodeInstruction`. (Except MCDXContainerStreamer,
>>> but I guess this method was overridden just for the vtable to be pinned to cpp file.)
>>
>> Actually, adding the logic to `MCObjectStreamer::emitInstructionImpl` was my original approach before sticking with the current version.
>> My previous attempt placed linker-relaxable instruction in its own fragment, instead of the end of a `MCDataFragment`.
>> However, it increased the number of fragments.
>
> Sadly you reconsidered, because I thought the same way.
>
>> Then I realized that the preferred solution is to place the linker-relaxable instruction at the end of a `MCDataFragment`.
>> With this choice, I don't think moving `DF->setLinkerRelaxable();` into `MCObjectStreamer::emitInstructionImpl` makes cleaner code.
>> I think it can be argued either way. Moving `DF->setLinkerRelaxable();` closer to `DF->setHasInstructions(STI);` has some value as well.
>
> Moving it into `emitInstructionImpl` would generalize the changes to non-ELF targets.

Unfortunately, RISC-V linker relaxation is very ELF-specific and I don't think other object file formats can benefit from a generalized interface.
LoongArch folks may copy the design (I am a bit concerned of this as the assembler infrastructure for linker relaxation needs significant re-architecture), otherwise no other ELF target will use the relaxation feature.
I think adding a `FKF_` flag is over-engineering without a clear benefit.

Moving the `setLinkerRelaxable` call into `MCObjectStreamer::emitInstructionImpl`, as you mentioned previously, would require significant refactoring to pull out `encodeInstruction` out of all derived `emitInstToData`.
(`MCDXContainerStreamer::emitInstToData` doesn't call `encodeInstruction`. The function is unfortunately not tested by `check-llvm`.)
Yet I don't see a value in terms of various aspects I can think of.



================
Comment at: llvm/lib/MC/MCELFStreamer.cpp:634
+    DF->setLinkerRelaxable();
   DF->getContents().append(Code.begin(), Code.end());
 
----------------
barannikov88 wrote:
> MaskRay wrote:
> > barannikov88 wrote:
> > > barannikov88 wrote:
> > > > MaskRay wrote:
> > > > > barannikov88 wrote:
> > > > > > * Maybe add a new fixup flag to `MCFixupKindInfo.h` and check if it is set?
> > > > > > * Can Fixups contain more than one element? If it can, why are you checking only the last one?
> > > > > > 
> > > > > Linker-relaxable instructions have two relocations and the second (last) one is `R_RISCV_RELAX`. There is just one relocation type, so adding a `FixupKindFlags` flag appears to be overkill. In addition, calling `getFixupKindInfo` adds some overhead to non-RISCV targets.
> > > > > 
> > > > > The current way causes the least overhead to non-RISCV targets.
> > > > > 
> > > > There is not much of overhead, but makes the solution scalable and avoids target-specific changes to common code.
> > > > 
> > > To support my point of view further, FKF_Constant is also used only by ARM backend.
> > `FKF_Constant` (my D72892) is probably not the best reference. I suspect that we can remove it but learning AArch32 is not my priority...
> > 
> > > makes the solution scalable and avoids target-specific changes to common code.
> > 
> > I am not sure I agree. Both `getTargetKind` and a `FixupKindFlags` leak into the generic code and do the same amount of "pollution" in my viewpoint. `FixupKindFlags` adds slightly more abstraction, so I'd avoid it.
> > `FKF_Constant` (my D72892) is probably not the best reference. I suspect that we can remove it but learning AArch32 is not my priority...
> > 
> > `FixupKindFlags` adds slightly more abstraction, so I'd avoid it.
> 
> Not sure I get it. Abstraction is almost always a win. Well, can't force you.
> 
> Well, can't force you.

Thank you:) I think the abstraction will just add a bit more cognitive load to readers. They will need to figure out that this is just about a specific fixup kind.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153097/new/

https://reviews.llvm.org/D153097