[PATCH] D93529: [AA] Cache (optionally) estimated PartialAlias offsets.

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.

I think the proper solution here would be to change `AliasResult` into a class that carries both the type of the result and an offset for PartialAlias (probably of reduced width), rather than making this information available via a cache side channel.

That would be a larger change though, and I'm okay with doing this in the meantime to make some progress.



================
Comment at: llvm/include/llvm/Analysis/AliasAnalysis.h:396
+      return Swapped ? -IOff->second : IOff->second;
+    }
+    return None;
----------------
nit: Unnecessary braces.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1184
+    const LocationSize *VLeftSize = &V2Size;
+    const LocationSize *VRightSize = &V1Size;
+
----------------
No need to use pointer for LocationSize, the type is okay to copy.


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1186
+
+    if (!Off.sge(0)) {
+      // Swap if we have the situation where:
----------------
nit: `Off.isNegative()`


================
Comment at: llvm/lib/Analysis/BasicAliasAnalysis.cpp:1199
+    if (VLeftSize->hasValue()) {
+      const auto LSize = VLeftSize->getValue();
+      if (Off.ult(LSize)) {
----------------
nit: auto -> uint64_t. Type is not obvious from context.


================
Comment at: llvm/unittests/Analysis/AliasAnalysisTest.cpp:323
+  if (!M)
+    Err.print("CodeMoverUtilsTests", errs());
+
----------------
Wrong test :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93529