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

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added a comment.
Herald added subscribers: wangpc, luke, wingo, sunshaoce, pmatos, arichardson.
Herald added a project: All.

In D116677#3241354 <https://reviews.llvm.org/D116677#3241354>, @MaskRay wrote:

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

I've opened #65479 <https://github.com/llvm/llvm-project/pull/65479> to implement this state idea and #65480 <https://github.com/llvm/llvm-project/pull/65480> to implement it for auipc+jalr on RISC-V.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116677