[PATCH] D52174: [TableGen][SubtargetEmitter] Add the ability for processor models to describe dependency breaking instructions.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 5 inline comments as done.
andreadb added inline comments.


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:292
+  // not exposed to the MC layer.
+  bit ExpandForMachineInstrOnly = machineInstrOnly;
+
----------------
courbet wrote:
> This feels like a double negation to me. What about "ExpandForMC" ? It would also make it more future-rpoof if we want to expand to more stuff.
Sure, I will change it. Thanks for the feedback!


================
Comment at: include/llvm/Target/TargetInstrPredicate.td:306
+//
+// If `Declaration.ExpandForMachineInstrOnly` is false, then SubtargetEmitter
+// will also expand another definition of this method that accepts a MCInst.
----------------
I will update this comment too.


================
Comment at: tools/llvm-mca/lib/InstrBuilder.cpp:458
+    if (IsDepBreaking) {
+      if ((Mask.isNullValue() && !RD.isImplicitRead()) ||
+          ((Mask.getBitWidth() > RD.UseIndex) && Mask[RD.UseIndex])) {
----------------
courbet wrote:
> This could use two temporary bool variables for readability.
I will change it.
I tried to expand that if-stmt a bit, adding extra code comments. Hopefully, it is more readable with code comments. Let me know if you prefer it done in a different way.


================
Comment at: utils/TableGen/CodeGenSchedule.cpp:257
+
+// Used by function `ProcessSTIPredicate` to construct a mask of machine
+// instruction operands.
----------------
courbet wrote:
> *processSTIPredicate
I will fix it. Thanks.


https://reviews.llvm.org/D52174