[PATCH] D41501: [Analysis] Support aggregate access types in TBAA

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:153
+      return false;
+    if (!isa<MDNodeTy>(Node->getOperand(0)))
+      return false;
----------------
Add a comment here reminding the reader that, in the old format, this first operand was a string.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:231
+    if (!isNewFormat())
+      return 0;
+    return mdconst::extract<ConstantInt>(Node->getOperand(3))->getZExtValue();
----------------
0 does not seem like a conservatively-correct result. I think this should either assert or return UINT64_MAX.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:272
+  /// size-aware format.
+  bool isNewFormat() const {
+    if (Node->getNumOperands() < 3)
----------------
This is the same as the function above. Maybe make a static helper function to avoid repeating the logic?


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:337
+    for (unsigned Idx = FirstFieldOpNo; Idx < Node->getNumOperands();
+             Idx += NumOpsPerField) {
       uint64_t Cur = mdconst::extract<ConstantInt>(Node->getOperand(Idx + 1))
----------------
Align this with the `unsigned` above.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:555
+    // fix this code to generate actual access sizes for generic tags.
+    uint64_t AccessSize = 0;
+    auto *SizeNode =
----------------
I think this should be UINT64_MAX, not 0, to be conservatively correct.  IR generated by this code might be interpreted later by an implementation that does care about the sizes.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:580
+
+static bool mayBeAccessToSubobjectOf(TBAAStructTagNode BaseTag,
+                                     TBAAStructTagNode SubobjectTag,
----------------
Please document the meaning of this function's parameters and return value.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:604
+
+  for(;;) {
+    // In the old format there is no distinction between fields and parent
----------------
Space after `for`.


================
Comment at: lib/Analysis/TypeBasedAliasAnalysis.cpp:608
+    if (!BaseType.getNode()) {
+      assert(!NewFormat);
+      break;
----------------
The assert should have a string that explains what's going on.


Repository:
  rL LLVM

https://reviews.llvm.org/D41501