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

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

I like the fact that we can now keep track of the implicit zero of the upper register bits.  Very cool.  This change makes sense, and as far as I can tell it looks good, but I'd wait for a few others to weigh in.



================
Comment at: lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:307
+public:
+  X86MCInstrAnalysis(const MCInstrInfo *MCII) : MCInstrAnalysis(MCII) {}
+
----------------
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.


================
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()));
----------------
Can we be sure that NumDefs <= BitVector::size?  I know Bitvector's access operators assert to check for out of bounds indexing.


================
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,
----------------
Should the names of the formal parameters be capitalized?  I realize you are trying to avoid clashing with the member names.


https://reviews.llvm.org/D48225