[PATCH] D69136: Add an instruction marker field to the ExtraInfo in MachineInstrs.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for putting up with these micro-optimized data structures, sorry they're opaque.



================
Comment at: llvm/lib/CodeGen/MachineInstr.cpp:549-550
+    if (memoperands_empty()) {
+      Info.set<EIIK_PreInstrSymbol>(getPreInstrSymbol());
+      Info.set<EIIK_PostInstrSymbol>(getPostInstrSymbol());
+      return;
----------------
rnk wrote:
> I don't think this does the right thing. If I understand correctly, `Info` is a single pointer. The .set call overwrites the pointer, and then the second call to getPostInstrSymbol will always return null, and then the .set call will reset the pointer to null again.
> 
> This extra info code is subtle. Now that there are five possibilities, we might want to come up with some shared logic for this.
> 
> Actually, I'm surprised we didn't write unit tests for this in the first place. Oops. It's highly testable data manipulation. You can see examples in llvm/unittests/CodeGen/MachineInstrTest.cpp.
I meant to elaborate on what I meant by "shared logic": I was thinking that all of the "extra info" accessors should call a routine that takes:
- an MMO ArrayRef
- a pre instr symbol
- a post instr symbol
- the new thing, DIType*, Metatadata*, or an ArrayRef<pair<unsigned, Metadata*>>

It then checks each case in turn:
- only one MMO and nothing else: use the EIIK_MMO case
- only one pre instr symbol: use the EIIK_PreInstr case
...
- if no cases apply, make the extra info struct


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69136