[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#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.

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.



================
Comment at: llvm/lib/MC/MCELFStreamer.cpp:634
+    DF->setLinkerRelaxable();
   DF->getContents().append(Code.begin(), Code.end());
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153097