[PATCH] D121508: [MCA] Moving six instruction flags out of InstrDesc and into InstructionBase.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

Just a couple of minor comments. Otherwise, patch looks good to me.

Thanks!



================
Comment at: llvm/include/llvm/MCA/CustomBehaviour.h:53-60
+  /// Some targets may wish to maintain some state within their IPP.
+  /// IPP is created in llvm-mca.cpp before we start working on any individual
+  /// code region. Because of this, if IPP maintains state, it will have its
+  /// state carry over between code regions. This is likely not desirable as
+  /// each region should be thought of as completely independent of the other
+  /// regions. The resetState() method gets invoked within llvm-mca.cpp at the
+  /// beginning of each code region so targets can override this function to
----------------
The last sentence alone is already descriptive enough. 

I suggest that we keep that last sentence only, and get rid of the reference to llvm-mca.cpp. While it is true that the llvm-mca driver is currently the only user of this logic (at least, this is true for upstream), there may be other downstream users of this library.


================
Comment at: llvm/include/llvm/MCA/Instruction.h:514-524
   bool IsALoadBarrier;
   bool IsAStoreBarrier;
 
+  // Flags copied from the InstrDesc and potentially modified by
+  // CustomBehaviour or (more likely) InstrPostProcess.
+  bool MayLoad;
+  bool MayStore;
----------------
Could you please convert all these eight boolean flags (starting from IsALoadBarrier, ending with your new flags) to bitfields?

Example:

```
bool IsALoadBarrier : 1;
bool IsAStoreBarrier : 1;
...
bool RetireOOO : 1;
```


================
Comment at: llvm/tools/llvm-mca/llvm-mca.cpp:516-518
+    // IPP may maintain state within a given code region, but since the IPP
+    // object persists between the different code regions, we should give it
+    // a chance to reset its state at the beginning of each region.
----------------
This comment is a bit repetitive and it can be removed. A similar comment is already added by this patch to the declaration of resetState. So, I am not convinced that it is adding extra information to the reader.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121508