[PATCH] D156158: [LAA] Rename and fix semantics of MaxSafeDepDistBytes to MinDepDistBytes

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM, thanks!



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2008
+    // couldPreventStoreLoadForward
+    assert(MinDepDistBytes == MinDepDistBytesOld &&
+           "An update to MinDepDistBytes requires an update to "
----------------
As `MinDepDistBytesOld` is only used in the assert, I think at the moment we would get a warning in release builds, so we probably either need `(void) MinDepDistBytesOld;` or wrap the declaration in an `ifdef`.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:2010
+  // since there is a backwards dependency.
+  uint64_t MaxVF = MinDepDistBytes / (TypeByteSize * Stride);
   LLVM_DEBUG(dbgs() << "LAA: Positive distance " << Val.getSExtValue()
----------------
michaelmaitland wrote:
> michaelmaitland wrote:
> > fhahn wrote:
> > > fhahn wrote:
> > > > This is the only non-NFC part of the patch, IIUC? Would it be possible to create a test case showing the new behavior?
> > > Would it be possible to add a test for the functional change here?
> > I assume you mean the fact that we update MaxSafeVectorWidthInBits even if `IsTrueDataDependence && EnableForwardingConflictDetection && couldPreventStoreLoadForward(Distance, TypeByteSize)`. 
> > 
> > I am working on constructing a test case for this.
> @fhahn I've updated this diff that no longer update MaxSafeVectorWidthInBits even if IsTrueDataDependence && EnableForwardingConflictDetection && couldPreventStoreLoadForward(Distance, TypeByteSize). The reason for this is that if `couldPreventStoreLoadForward` returns true, then it must be the case that `MinDepDistBytes` was not updated, and therefore MaxSafeVectorWidthInBits will remain the same.
> 
> I've added an assert statement as a sanity check against any future changes.
Ok thanks! I think that is fine for now, as this keeps the logic in this part working as before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156158