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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added a comment.

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.



================
Comment at: llvm/include/llvm/MC/MCFragment.h:251
+
+  bool getLinkerRelaxable() const { return LinkerRelaxable; }
+  void setLinkerRelaxable() { LinkerRelaxable = true; }
----------------
A brief comment would be nice.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153097