[PATCH] D65948: [MCA] Add flag -show-encoding to llvm-mca.

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet accepted this revision.
gchatelet added inline comments.


================
Comment at: lib/MCA/CodeEmitter.cpp:20
+CodeEmitter::getOrCreateEncodingInfo(const MCInst &Inst) {
+  auto It = EncodingRefs.find(&Inst);
+  if (It != EncodingRefs.end())
----------------
andreadb wrote:
> gchatelet wrote:
> > I'm not a huge fan of using the address of `MCInst` as the key in the cache, for instance the following code would be buggy:
> > ```
> > CodeEmitter CE;
> > MCInst Inst = GetInst();
> > CE.getOrCreateEncodingInfo(Inst);
> > Inst = GetAnotherInst();
> > CE.getOrCreateEncodingInfo(Inst);
> > 
> > ```
> > 
> > If you still want to use the address for performance reasons you can pass by pointer instead of reference. At least this will be a red flag,
> Yes it would be bugged for that case.
> 
> However, that case is not going to happen in MCA because the sequence of MCInst is constructed once at the beginning, and then it is never changed. It is passed around as an ArrayRef and no module is meant to change any MCInsts or add extra elements to the sequence.
> That sequence is never invalidated nor changed during the entire execution.
> 
> So yes, I totally agree with you that using a pointer is ugly and scary. But in practice - for our particular scenario - it works in a fast and easy.
> 
> That being said, I could use an instruction index.
> However, that would complicate future patches to the InstrBuilder (which doesn't know anything about the instruction index). I'll see if I can use the instruction index and avoid any controversial change :).
> 
It looks much better now IMHO : ) Thx for taking the time to improve on it.


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

https://reviews.llvm.org/D65948