[PATCH] D83283: [Attributor] AAPotentialValues Interface

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Apologies for the long wait. Here are some more comments.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3156
+  /// When \p isAtWorstState is false, this corresponds to the actual set.
+  SetTy Set;
+};
----------------
Make the two members private please, we can access them via getters.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3181
+    return !Assumed.isAtWorstState && Assumed.Set.size() < MaxPotentialValues;
+  }
+
----------------
I would prefer if we update `isAtWorstState` as soon as we modify the set and reach the number of allowed values. So after each (potential) increase we check and update `isAtWorstState`. That way it is clear that "isValid" always equals "isAtWorstState". (We can also name `isAtWorstState` `IsValid`.)


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3184
+  /// See AbstractState::isAtFixpoint()
+  bool isAtFixpoint() const override { return Assumed.isAtWorstState; }
+
----------------
This "tracks" only the worst state fixpoint but not any other one, e.g., if it is a known constant. Are you sure we don't need a separate boolean to track this. I would have assumes so to avoid calling `update` on the AA after we used `indicateOptimisticFixpoint`.

One way to handle this is to replace `isAtWorstState` with a `BooleanState` which handles the fixpoint and the valid bit. You can probably even derive from that class to minimize the code you need to type, e.g., a lot of functions provided by `BooleanState` might then just be good enough. Only once you need to inspect or touch the Set you have to provide functions here.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3210
+  /// Note: If Assumed.isAtWorstState is true, this is meaningless.
+  SetTy getAssumedSet() const { return Assumed.Set; }
+
----------------
We might want to add an assertion here. no point in handing out meaningless data. (I guess other types lack such an assertion as well.)


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:3277
+    if (Assumed.isAtWorstState)
+      return nullptr;
+    if (getAssumedSet().size() == 1)
----------------
We should not have a variable like this but use `!Assumed.isValid()` for this.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7250
+  }
+};
+
----------------
Would it make sense to let `AAPotentialValuesCallSiteArgument` derive from `AAPotentialValuesFloating`? We often do that.


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

https://reviews.llvm.org/D83283