[PATCH] D150004: [RISCV][MC] .debug_line/.debug_frame/.eh_frame: emit relocations for assembly input files with relaxation

Sat May 13 12:08:03 PDT 2023

MaskRay added a comment.

In D150004#4339796 <https://reviews.llvm.org/D150004#4339796>, @barannikov88 wrote:

> I did more digging and found two related patches https://reviews.llvm.org/D45181 and https://reviews.llvm.org/D61584.
> The third patch, already mentioned here (https://reviews.llvm.org/D103539)
> effectively undoes the first two. This may justify why it removed canFold.
> I also like the direction taken in the third patch - push as much as possible
> out of generic code.

Thanks for investigating! Knowing the two related patches will be useful and give more clue for future incremental improvement.

> ---
>
> In D150004#4339342 <https://reviews.llvm.org/D150004#4339342>, @MaskRay wrote:
>
>> It's unfortunate that you feel the need to retreat. Could you please elaborate on the specific points that left you unconvinced? Otherwise there is a risk of causing confusion or misunderstanding.
>
> The thing that concerns me is that evaluateAs* methods do not behave
> as expected. They may "incorrectly" fold expressions if asm layout is provided.
> This functionality either should be removed completely, or the methods
> should be taught not to fold expressions when it is wrong to do so.

Yes, `evaluateAs*` methods need to be fixed. See the last paragraph of this comment.

> This patch goes in the middle by avoiding calls, which I don't like.
>
>> The `evaluateAsAbsolute` code is false for "direct object emission".
>
> is false because it returns true for constant expressions. I don't see why we
> should take the slow path in this case. 2% degradation out of the blue does
> not help justifying this change.

The "slow path" is slow for assembly input. By removing the "fast path," we can slightly improve the speed of "direct object emission" since we save time on a path that is not expected to be taken.

Therefore, I question the merit of retaining the "fast path."
In my conclusion, it is not necessary (https://reviews.llvm.org/D150004#4339113), at least not for now[now].

`MCDwarfLineAddr::Emit` and `MCDwarfFrameEmitter::encodeAdvanceLoc` do complex operations that are repeated in `MCAssembler::relaxDwarfLineAddr` and `MCAssembler::relaxDwarfCallFrameFragment`.
The latter two have intricate RISCV overrides.

There are opportunities to make the "slow path" faster, e.g.

- Optimizing the current `new MC*Fragment` pattern that allocates new fragments on the heap.
- Reducing the number of relaxation times for `.debug_line` and `.debug_frame`, as well as possibly other sections using LEB128. For instance, LEB128 can have a one-byte estimate to avoid the second relaxation iteration.

As a result, the benefits of the "fast path" (which only applies to assembly input) may become even smaller.

[now]: I mentioned this as a temporary decision. After revisiting how the RISC-V overrides should be implemented, there might still be a chance to reintroduce the "fast path" just for assembly input.
I can create an issue (https://github.com/llvm/llvm-project/issues) in case it is overlooked.

>> Eliminating `evaluateAsAbsolute` with a `MCAssembler` uses, if the performance overhead is acceptable, is the preferred approach IMO.
>
> IMO the preferred approach is to not remove the call, but rather remove
> the argument that allows generating invalid code. Or, teach this method
> not to generate invalid code. This may be as simple as adding a boolean
> flag that forbids folding, but this looks equivalent to removing the
> Layout argument.

Are you suggesting to remove the getAssemblerPtr() argument? AIUI this would prevent the fast path from ever being triggered, even for assembly input.

>> The fast path of `MCObjectStreamer::emitValueImpl` does a simple operation, so keeping the fast path is more justified.
>
> I do not agree. It may be more justified from the performance point of view,
> but not from the correctness point of vew. I believe correctness should be
> the foremost factor. If it was correct, RISC-V wouldn't need to override it.
>
>> Furthermore, there is a concern that making the ambitious changes might introduce further fragility to linker relaxation infrastructure.
>
> This statement needs some support. I could as well say that it might make linker
> relaxation infrastructure more robust.
>
>> I have tried reading MCExpr.cpp multiple times and I always feel that some parts of it need more love to address some problems (some are documented as FIXME).
>
> Here, I totally agree. evaluateAs* methods are broken, and we should fix them rather than avoid using them.

Instead of D150373 <https://reviews.llvm.org/D150373> (which reintroduces D61584 <https://reviews.llvm.org/D61584>), I am considering an ambitious refactoring that involves separating relaxable instructions into their own fragment.
This is how x86 JMP/JCC/etc are relaxed.

Currently, relaxable and non-relaxable instructions are placed together in one MCDataFragment. However, this is not the intended design of MCDataFragment.
By placing relaxable instructions in their own fragments, we can utilize the AttemptToFoldSymbolOffsetDifference generic code to avoid folding for assembly input.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150004