[PATCH] D79351: [MCA] Fixed a bug where loads and stores were sometimes incorrectly marked as depedent (PR45793).

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 6 inline comments as done.
andreadb added a comment.

In D79351#2018657 <https://reviews.llvm.org/D79351#2018657>, @mattd wrote:

> I'm totally cool with this change; however, it's been a while since I've taken a look at this part of MCA.  I'll let other's chime in as well, but +1 from me.


Thanks Matt :-)



================
Comment at: llvm/include/llvm/MCA/HardwareUnits/LSUnit.h:61
 
-  ArrayRef<MemoryGroup *> getSuccessors() const { return Succ; }
-  unsigned getNumSuccessors() const { return Succ.size(); }
+  unsigned getNumSuccessors() const {
+    return OrderSucc.size() + DataSucc.size();
----------------
mattd wrote:
> s/unsigned/size_t/  because SmallVectorBase::size returns a size_t.
Right. Ideally it should return a size_t. I guess it hardly would be a problem in practice. But you are right. I will fix it.


================
Comment at: llvm/include/llvm/MCA/HardwareUnits/LSUnit.h:119
 
+    if (!ShouldUpdateCriticalDep)
+      return;
----------------
To clarify.
The reason why there is this early exit here is because we only need to update the information about a critical "memory" dependency if there is memory aliasing.


================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:69
 
 unsigned LSUnit::dispatch(const InstRef &IR) {
   const InstrDesc &Desc = IR.getInstruction()->getDesc();
----------------
mattd wrote:
> This function is getting pretty large.  I'm not sure how to break this up, but it could be useful as a NFC follow-up at a later time.
Definitely a good suggestion for a follow-up. :-)
This logic can probably be split and moved into a few helper methods/functions.



================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:95
+    // A store may not pass a previous store barrier.
+    if (CurrentStoreBarrierGroupID) {
+      MemoryGroup &StoreGroup = getGroup(CurrentStoreBarrierGroupID);
----------------
Note that, by construction `CurrentStoreBarrierGroupID` is always less than or equal to `CurrentStoreGroupID`.
This check was missing in the original code from commit 280ac1fd1dc35. And this is exactly the reason why we have a difference in test pr37790.s


================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:105
+    if (CurrentStoreGroupID &&
+        (CurrentStoreGroupID != CurrentStoreBarrierGroupID)) {
       MemoryGroup &StoreGroup = getGroup(CurrentStoreGroupID);
----------------
This check is to avoid that we add a store dependency twice (since a store can also be a store barrier).


================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:131
+
+  bool ShouldCreateANewGroup = IsMemBarrier || !ImmediateLoadDominator ||
+                               ImmediateLoadDominator <= CurrentStoreGroupID ||
----------------
mattd wrote:
> Is !ImmediateLoadDominator necessary here?  It seems that would be implied by the next condition:  `ImmediateLoadDominator <= CurrentStoreGroupID`  However, I suppose this does read clearer by leaving the !ImmediateLoadDominator check in place..
The idea is to identify cases where we need a new node in the dependency graph.

These are the cases identified by this check:
 a) This is a memory barrier (by construction we always require that barriers are assigned to different memory group);
 b) This is the very first load dispatched to the LSUnit (by construction we always keep loads and stores into separate groups. So we need to start a new load group).
 c) There is an intervening store between the last load dispatched to the LSU and this load;
 d) There is no intervening store. However the last load group has already started execution (so we need a new node).



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

https://reviews.llvm.org/D79351