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

Mon May 4 14:00:15 PDT 2020

mattd added inline comments.

================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:96
+    if (CurrentStoreBarrierGroupID) {
+      MemoryGroup &StoreGroup = getGroup(CurrentStoreBarrierGroupID);
+      LLVM_DEBUG(dbgs() << "[LSUnit]: GROUP DEP: ("
----------------
nit: You call this StoreGroup here, but StGroup a few blocks below.  I really do not care that there is a difference of a few characters, but I could not silence my inner dialog.

================
Comment at: llvm/lib/MCA/HardwareUnits/LSUnit.cpp:131
+
+  bool ShouldCreateANewGroup = IsMemBarrier || !ImmediateLoadDominator ||
+                               ImmediateLoadDominator <= CurrentStoreGroupID ||
----------------
andreadb wrote:
> 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).
> 
Those points would make for a great comment!  Thanks for the clarification.

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

https://reviews.llvm.org/D79351