[PATCH] D48225: [llvm-mca][X86] Teach how to identify register writes that implicitly clear the upper portion of a super-register.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:307
+public:
+  X86MCInstrAnalysis(const MCInstrInfo *MCII) : MCInstrAnalysis(MCII) {}
+
----------------
mattd wrote:
> Since you have a static routine for creating instances of this class: `createX86MCInstrAnalysis`, you can probably make this ctor private.  It can probably be marked `explicit`, to imply no converting.
Unless I am missing something, the `createX86MCInstrAnalysis` static routine would not be able to see the constructor if we make it private.


================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:328
+      const MCOperand &Op = Inst.getOperand(I);
+      Writes[I] = GR32RC.contains(Op.getReg()) ||
+                  (HasVEXOrEVEX && XMMRC.contains(Op.getReg()));
----------------
mattd wrote:
> Can we be sure that NumDefs <= BitVector::size?  I know Bitvector's access operators assert to check for out of bounds indexing.
We cannot be sure. We assume that it has been set to the right size by the user.
Otherwise, it will assert inside BitVector.
What we could do, is to always "resize" the BitVector to the actual number of register writes (from the information in Desc).


================
Comment at: tools/llvm-mca/InstrBuilder.h:55
 public:
-  InstrBuilder(const llvm::MCSubtargetInfo &sti, const llvm::MCInstrInfo &mcii)
-      : STI(sti), MCII(mcii),
+  InstrBuilder(const llvm::MCSubtargetInfo &sti, const llvm::MCInstrInfo &mcii,
+               const llvm::MCRegisterInfo &mri,
----------------
mattd wrote:
> Should the names of the formal parameters be capitalized?  I realize you are trying to avoid clashing with the member names.
I have seen it done in a few (not many) places in LLVM specifically to avoid the issue with name clash. I can use different names to avoid the name clashing. Alternatively, I just capitalise the param names; it would still work, but it would not be nice to read.


https://reviews.llvm.org/D48225