[PATCH] D83744: [Attributor] Attributor call site specific AAValueConstantRange

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some minor comments below.

Upload diffs with full context please.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:543
+          .getAAFor<AAType>(QueryingAttribute, IRPosition::value(*Val))
+          .getState();
+
----------------
Can we split this in two statements for readability. Also use a C++ cast please. I think we can actually do `cast` on the AA and then getState() should give the right type. We might need to adjust the state wrapper return type though. Hope this makes some sense.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:574
     clampCallSiteArgumentStates<AAType, StateType>(A, *this, S);
+
     // TODO: If we know we visited all incoming values, thus no are assumed
----------------
`Success`


================
Comment at: llvm/test/Transforms/Attributor/cb_range.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
+; RUN: opt -S -aa-pipeline=basic-aa -passes=attributor --attributor-enable-call-site-specific=true | FileCheck %s -check-prefix=CHECK
+
----------------
uenoku wrote:
> Could you run for the case of --attributor-enable-call-site-specific=false as a comparison?
we should do this for the old and new PM, as well as the module and CGSCC pass.
so take the 4 run lines we have in other tests, duplicate them so you have them once w/ and once w/o the flag. The check prefixes might need to be adjusted in that case but basically you want check prefixes to overlap so if the code is the same it is shown in the check lines right away.


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

https://reviews.llvm.org/D83744