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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

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)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116677