[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Last set of minor comments. I think this is ready in after.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3075
+/// Provide DenseMapInfo for APInt.
+struct DenseMapAPIntKeyInfo {
+  static inline APInt getEmptyKey() {
----------------
Make this
```
template<>
struct DenseMapInfo<APInt> {
```


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3109
+
+  PotentialValuesState() : BooleanState(false) {}
+
----------------
The default state should not force the boolean state to "invalid". Just keep the default behavior which is to create an optimistic state by default.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3113
+
+  PotentialValuesState(const SetTy &PS) : BooleanState(true) { Set = PS; }
+
----------------
Do we need this?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3120
+    return Set;
+  }
+
----------------
Make the return type `const SetTy &`. No need to copy the set always, if the caller wants to do that they still can.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3149
+  /// Union assumed set with the passed value.
+  void unionAssumed(const APInt &C) { insert(C); }
+
----------------
APInt should be `MemberTy`, right?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3196
+      if (!checkAndInvalidate())
+        return;
+    }
----------------
Just check at the end once.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3210
+    }
+    SetTy intersectSet;
+    for (const MemberTy &C : Set) {
----------------
Nit: `IntersectSet`


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3241
+///
+/// TODO: Support other values than integers as well.
+struct AAPotentialValues
----------------
Nit: `TODO: Support values other than constant integers.`


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7101
+    return OS.str();
+  }
+
----------------
I guess something like this should be the default implementation in AbstractAttribute already. We could probably remove quite a lot of duplication. Follow up ;)


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7246
+    }
+  }
+
----------------
We don't need this method (for now). The one in `AAPotentialValuesFloating` will do fine (for now).


================
Comment at: llvm/test/Transforms/Attributor/potential.ll:385
+  ret i1 %ret
+}
----------------
Manually add check lines for the ranges please.


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

https://reviews.llvm.org/D83283