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

Sat May 13 02:20:47 PDT 2023

barannikov88 added a comment.

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.

---

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

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150004