[PATCH] D22634: [CFLAA] Add support for field offset in CFLAnders::FunctionInfo

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!


================
Comment at: lib/Analysis/AliasAnalysisSummary.h:140
@@ +139,3 @@
+// because we require a signed value.
+static LLVM_CONSTEXPR int64_t UnknownOffset =
+    std::numeric_limits<int64_t>::max();
----------------
Please rebase; it looks like my intuition on your prior patch was only half-correct. ;)

We apparently have a bot that uses clang on Windows, with earlier versions of MS's stdlib. So, `LLVM_CONSTEXPR` becomes `constexpr`, and `max()` still isn't `constexpr` on that particular bot. My bad.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:499
@@ +498,3 @@
+    // Find out all (X, Offset) where X == RHS
+    auto RangePair = std::equal_range(
+        Itr->second.begin(), Itr->second.end(), OffsetValue{RHS, 0},
----------------
Given that we're using a different comparator than the one we sorted with here, can we add the following above this line, please?

```
#ifdef EXPENSIVE_CHECKS
assert(std::is_sorted(Itr->second.begin(), Itr->second.end(), /* your comparator here */));
#endif
```

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:521
@@ +520,3 @@
+
+        // FIXME: What happens when LHSSize or RHSSize is larger than INT64_MAX?
+        auto LHSStart = OVal.Offset;
----------------
Pick your favorite:

`assert(LHSSize <= INT64_MAX)`

or

`if (LLVM_UNLIKELY(LHSSize > INT64_MAX || RHSSize > INT64_MAX)) return MayAlias;`

:)

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:523
@@ +522,3 @@
+        auto LHSStart = OVal.Offset;
+        auto LHSEnd = OVal.Offset + static_cast<int64_t>(LHSSize);
+        auto RHSStart = 0;
----------------
If we care about potential overflow, this may be a good place to put a FIXME, as well.

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:527
@@ +526,3 @@
+        if (LHSEnd > RHSStart && LHSStart < RHSEnd)
+          return (LHSStart == 0) ? MayAlias : PartialAlias;
+      }
----------------
`PartialAlias` doesn't seem quite right here. `PartialAlias` is essentially `MustAlias` (at some offset, as you've noted), and, unless I'm missing something obvious, I think we'd need to do many more checks to prove `MustAlias`.

(If I *am* wrong, s/`MayAlias`/`MustAlias`/, please)

================
Comment at: lib/Analysis/CFLAndersAliasAnalysis.cpp:537
@@ -440,2 +536,3 @@
 
   if (AttrsA.none() || AttrsB.none())
+    return NoAlias;
----------------
Given that these checks are relatively cheap, can we put them before our `AliasMap` checks (which seem potentially more expensive)?


https://reviews.llvm.org/D22634