[PATCH] D49196: [llvm-mca][BtVer2] teach how to identify false dependencies on partially written registers.

[Matt Davis via Phabricator via llvm-commits]

mattd added inline comments.


================
Comment at: include/llvm/Target/TargetSchedule.td:506
+// different physical registers. However, there is a cost to pay for when the
+// partial write is combined wit the previous super-register definition.
+// We should add support for these cases, and correctly model merge problems
----------------
s/wit/with/


================
Comment at: tools/llvm-mca/Instruction.cpp:143
+      int WriteLatency = Write->getCyclesLeft();
+      if (Write->getCyclesLeft() == UNKNOWN_CYCLES)
+        return false;
----------------
Style: You could just use WriteLatency in place of  the call to getCyclesLeft.


================
Comment at: tools/llvm-mca/RegisterFile.cpp:29
                            const llvm::MCRegisterInfo &mri, unsigned NumRegs)
-    : MRI(mri), RegisterMappings(mri.getNumRegs(), {WriteRef(), {0, 0}}) {
+    : MRI(mri), RegisterMappings(mri.getNumRegs(), {WriteRef(), {{0, 1U}, 0}}) {
   initialize(SM, NumRegs);
----------------
I think you can treat the {0,1U} as a call to the IndexPlusCostPairTy constructor.  That would be a bit easier to read.


================
Comment at: tools/llvm-mca/RegisterFile.cpp:99
+        RegisterRenamingInfo &OtherEntry = RegisterMappings[*I].second;
+        if (!OtherEntry.IndexPlusCost.first &&
+            (!OtherEntry.RenameAs ||
----------------
We should note that index 0 is a special case, and probably have a similar comment in RegisterFile.h where IndexPlusCostPairTy is declared.


================
Comment at: tools/llvm-mca/RegisterFile.cpp:153
+
+
+  RegisterRenamingInfo &RRI = RegisterMappings[RegID].second;
----------------
Style: Remove a line of white space.


================
Comment at: tools/llvm-mca/RegisterFile.cpp:243
   for (MCSuperRegIterator I(RegID, &MRI); I.isValid(); ++I) {
-    WR = RegisterMappings[*I].first;
-    if (WR.getWriteState() == &WS)
-      WR.invalidate();
+    WriteRef &OtherWR = RegisterMappings[*I].first;
+    if (OtherWR.getWriteState() == &WS)
----------------
This block looks duplicated from line 233 above.  It looks like we will invalidate the supers of RegID twice if WS are clearing super regs. 


================
Comment at: tools/llvm-mca/RegisterFile.h:155
   // files are available.  Otherwise, the mask can be used to identify which
-  // register file was busy.  This sematic allows us classify dispatch dispatch
+  // register file was busy.  This sematic allows us classify dispatch
   // stalls caused by the lack of register file resources.
----------------
Add 'to' to the following phrase: ... allows us to classify ...


https://reviews.llvm.org/D49196