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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

(Tried to get this out last weekend, but was blocked by the Phab down time... Sorry about that ...)

Mostly nits around the exact code here. The approach looks really nice now (and sorry it took so many iterations to get there).



================
Comment at: lib/Passes/PassRegistry.def:188
 FUNCTION_PASS("lower-guard-intrinsic", LowerGuardIntrinsicPass())
+FUNCTION_PASS("lower-is-constant", LowerConstantIntrinsicsPass())
 FUNCTION_PASS("lower-widenable-condition", LowerWidenableConditionPass())
----------------
Maybe `lower-constant-intrinsics` as a name? (Since it handles `objectsize` as well.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:93
+    for (Instruction &I: *BB) {
+      if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I)) {
+        switch (II->getIntrinsicID()) {
----------------
Use an early continue to reduce indentation?


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:96
+        default:
+          continue;
+        case Intrinsic::is_constant:
----------------
Odd to continue here but break below. Doesn't matter in this case of course, but just seemed surprising.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:105-106
+  }
+  if (Worklist.empty())
+    return false;
+
----------------
FWIW, this doesn't skip anything, the loop has the same behavior.


================
Comment at: lib/Transforms/Scalar/LowerConstantIntrinsics.cpp:112-117
+    if (!II)
+      continue;
+    Value *NewValue;
+    switch (II->getIntrinsicID()) {
+    default:
+      continue;
----------------
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.


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

https://reviews.llvm.org/D65280