[PATCH] D116677: [2/4] [MC][NFC] Add an optional PreviousInst argument to MCInstrAnalysis::evaluateBranch

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D116677#3240206 <https://reviews.llvm.org/D116677#3240206>, @asb wrote:

> Thanks for the review @MaskRay, I had thought about options such as adding state to MCInstrAnalysis, but perhaps was too quick to dismiss it (obviously partly influenced by the fact that the current RISC-V backend doesn't seem to generate output where just checking instruction pairs doesn't work!). As you suggest, an issue will be around tracking/clearing state upon basic block boundaries, but perhaps a simple heuristic will work well enough.
>
> So perhaps:
>
> - Keep the existing evaluateBranch interface (i.e. pass a single MCInst)
> - Add a reset() method to MCInstrAnalysis to allow users to reset state as desired
> - Adding state to RISCVMCInstrAnalysis, and having evaluateBranch manipulate it.
> - Have evaluateBranch reset the state whenever encountering a control flow instruction. This is imprecise - it will be overly conservative in some potential cases and too restrictive in others, but hopefully on real-world code it means simple cases are appropriately annotated, and there are few/no instances of confusing, incorrect output.
>
> What do you think? (Especially on the last bulletpoint)?

Sounds good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116677