[PATCH] D71620: [Attributor] AAValueConstantRange: Value range analysis using constant range

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think the final round of comments.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4905
+    return ConstantRange::getFull(getBitWidth());
+  }
+
----------------
Nit: I'd move this into the state as a static member (`getWorstState`) taking a bit width. That makes it consistent with the IntegerStates.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4980
+    return getKnown().intersectWith(SCEVR).intersectWith(LVIR);
+  }
+
----------------
Shouldn't we merge the SCEV and LVI information into the known state during intialization and then be done with it?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5001
+    return getAssumed().intersectWith(SCEVR).intersectWith(LVIR);
+  }
+
----------------
As mentioned above, if we merge the SCEV and LVI results into known there is no need to do anything but return the assumed value. The TODO should stay though.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5054
+    return false;
+  }
+
----------------
I don't think we have a test which checks the metadata, right? We should create one if possible or add a TODO here explaining why it's not possible yet.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5217
+        if (this == &LHSAA || this == &RHSAA)
+          return false;
+
----------------
I doubt this works to prevent PHI circles. If you have `%phi = phi [0, ...], [%bo1, ...]; ...; %bo0 = add %phi, 1; ...; %bo1 = add %bo0, %bo0` there is a circle but of size 3, which should not cause this check to trigger.

That said, do we really need it? If so, do we have a test to show that?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5226
+        if (LHSAA.isAtFixpoint() && RHSAA.isAtFixpoint() && !Stripped)
+          T.indicateOptimisticFixpoint();
+
----------------
I don't think we should do that. The Attributor will fix states without dependences and this seems risky given that the genericValueTraversal could look phi nodes and selects and stuff.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:5277
+        if (LHSAA.isAtFixpoint() && RHSAA.isAtFixpoint() && !Stripped)
+          T.indicateOptimisticFixpoint();
+
----------------
As above.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:6181
+  (void)Success;
+  assert(Success && !AnyDead && "Expected the check call to be successful!");
 }
----------------
I'm not even sure we need this. Shouldn't AAValueSimplify call AAValueConstantRange if necessary?
Instead, I would create AAValueConstantRange for calls with integer return type (and add a TODO for load instructions). Here we can come up with metadata which is otherwise not derived.


================
Comment at: llvm/test/Transforms/Attributor/lvi-after-jumpthreading.ll:34
+  ret i8 0
+}
+
----------------
I know I suggested this test but now I wonder:
The LVI test prints some internal stuff after it runs jump-threading, which we have not integrated yet. We should either add a FIXME to integrate jump-threading, add some way of testing the range analysis e.g., through an array access that we can prove dereferenceable, or remove the test.

Now after writing this I realize we actually optimize the branch away! So we should keep the test but maybe add a pointer access with a FIXME for dereferencebaility anyway somewhere.


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

https://reviews.llvm.org/D71620