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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
