[PATCH] D158280: [jitlink/rtdydl][checker] Construct disassembler at every decodeInst

Mon Sep 4 06:48:04 PDT 2023

sgraenitz added a comment.

Sorry for being late at the party. I was too busy in the last weeks to approach non-trivial reviews. This is looking really good already. Thanks for your help @lhames!!
Here are my 2 cents. Unfortunately, I cannot make inline comments anymore.. might be due to the move from Pharicator to GitHub PRs <https://discourse.llvm.org/t/pull-request-migration-reminder-sept-1-oct-1/73043>. I will do my best to keep this readable.

-----

> Eymay wrote:
> Maybe getDisassembler and getInstrPrinter could share code for MRI and MAI?
>
> lhames wrote:
> Looks like MCContext doesn't take ownership -- we're probably leaking them at the moment.
> I don't think I'd worry about sharing them (at least not yet -- we could do it as an optimization later). For now I'd be inclined to add a struct to hold on to the MRI and MAI (and any other types that aren't owned) alongside the disassembler / inst-printer.

Agree with @lhames: The code sharing is optional, be we must prevent `STI`, `MRI`, `MAI` and `Ctx` from getting destroyed once their `unique_ptr`s go out of scope, because `Disassembler` only takes raw pointers and no ownership. (Same for `getInstPrinter` with `MRI`, `MAI`, `MII` and `InstPrinter`). I see two options:
(1) Define a `struct` with fields for each of them (plus the Disassembler/InstPrinter itself) and return that. This is what Lang was saying.
(2) Inline the code into the calling functions.

I think (1) is clean but adds more effort. (2) is easier, but it would impact the readability of the `evalDecodeOperand` function significantly. You could consider to outline the respective error reporting code from `evalDecodeOperand` (from `auto InstPrinter` to `return std::make_pair([...])`) and put the `getInstrPrinter` code in there. I don't see a problem putting the `getDisassembler` code directly into `decodeInst`.

-----

In `RuntimeDyldChecker` is there a good reason to have these members them as references? Otherwise let's store them as values, pass them to the ctor by value and forward with `std::move`.

  Triple &TT;
  SubtargetFeatures &TF;

-----

> Eymay wrote:
> I suggest in case of failure the instPrinter can try alternative TargetFlag and give diagnostic about the decoded instruction together with an error message of TargetFlag setting.

If that adds significant complexity, then why not keep it as a potential future follow-up? I think the scope of this patch is good as it is now.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158280