[Lldb-commits] [PATCH] D69210: [Disassembler] Simplify MCInst predicates

[Vedant Kumar via Phabricator via lldb-commits]

vsk marked an inline comment as done.
vsk added a comment.

In D69210#1716861 <https://reviews.llvm.org/D69210#1716861>, @clayborg wrote:

> In D69210#1715679 <https://reviews.llvm.org/D69210#1715679>, @vsk wrote:
>
> > Hm, this patch is bugging me.
> >
> > It looks a bit like instructions are still decoded multiple times in different ways (e.g. in the `Decode` and `CalculateMnemonicOperandsAndComment` methods, which both modify `m_opcode`). Any ideas on whether/how to consolidate these?
>
>
> I am all for anything that will improve efficiency. This class has evolved over time where we started with just the "CalculateMnemonicOperandsAndComment" and then many other features (can branch, etc) were later built into the class. I don't believe instructions are kept around for long so they typically serve one of two purposes:
>
> - disassembly of instruction stream where only CalculateMnemonicOperandsAndComment is needed
> - inspection of multiple instructions for stepping looking at can branch and other information requests
>
>   So I am not sure the decoded multiple times in different ways is really important unless we do have a costly client that does both CalculateMnemonicOperandsAndComment and inspecting of instruction attributes (can branch, etc). Again, these objects are created, used and discarded currently AFAIK.


Thanks for your comment Greg. Let me try and restate the issue I see as my concern isn't about performance.

It looks like `Decode` and `CalculateMnemonicOperandsAndComment` mutate `m_opcode` in different ways. Separately, the predicates read `m_opcode`. So I'm not sure whether/in-which-order the mutating methods need to be run before the predicates can safely be called. I'd like to consolidate all the code that mutates `m_opcode` in one place, to make the predicates always safe to call. Does that seem reasonable? Or am I overthinking something?



================
Comment at: lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp:87
+    //   - Is not a call
+    m_does_branch = eLazyBoolYes;
+    m_has_delay_slot = eLazyBoolNo;
----------------
clayborg wrote:
> Why init this to eLazyBoolYes?
I believe this preserves the existing behavior of the class. InstructionLLVMC conservatively says that instructions can branch, in the absence of information.


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

https://reviews.llvm.org/D69210