[PATCH] D65280: Add a pass to lower is.constant and objectsize intrinsics

[Joerg Sonnenberger via Phabricator via llvm-commits]

joerg added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:105-106
+  }
+  if (Worklist.empty())
+    return false;
+
----------------
chandlerc wrote:
> FWIW, this doesn't skip anything, the loop has the same behavior.
It was primarily to get the correct return value, but I'm changing it to push the check to the final return.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:112-117
+    if (!II)
+      continue;
+    Value *NewValue;
+    switch (II->getIntrinsicID()) {
+    default:
+      continue;
----------------
chandlerc wrote:
> For both the `II` thing and the `default` case -- do we really expect these to ever fail?
> 
> I would expect either the VH to be null, or for it to definitively be one of the two intrinsics we added. Maybe switch to `cast_or_null` above with `VN.get()` or some such, and llvm_unreachable on the default case.
Yes, the same concerns as with the earlier version still apply. The recursive simplification can change the instruction type in place or remove it. The logic is still simpler since no new instructions can appear.


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

https://reviews.llvm.org/D65280