[PATCH] D64963: Add a pass to lower is.constant intrinsics

[Bill Wendling via Phabricator via llvm-commits]

void marked 2 inline comments as done.
void added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp:9
+//
+// This pass lowers the 'is.constant' intrinsic to 'true' or 'false'.
+//
----------------
joerg wrote:
> Fundamentally, we don't have to care too much about the 'true' part. That's handled by normal constant folding well enough already. It's only a fallback for the -O0 pass really.
I was mostly thinking about the `-O0` pass, as you mentioned. But yeah we don't need to worry too much about it. Though I agree with Chandler (I think he was the one who mentioned it) that we would like to convert to `false` as late as possible and `true` as early as possible.


================
Comment at: lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp:44
+  if (Exp) {
+    II->replaceAllUsesWith(Exp);
+    II->eraseFromParent();
----------------
joerg wrote:
> RAUW is not good enough here. We want to have at least replaceAndRecursivelySimplify, but even that is not good enough as it still leaves conditional branches on constant conditions. D62025 depended on SimplifyCFG for that, but that doesn't work for the -O0 chain. That said, is there a reason for not handling that in InstructionSimplify.cpp explicitly?
Not handling which case in InstructionSimplify.cpp?

I can change the call, but I want to be wary of "separation of concerns". Normally, we just change the instruction and let other passes do the clean up. I don't want to put a whole lot of logic in here to DCE, simplify, etc., unless we know those passes won't run afterwards.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64963