[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:
> void wrote:
> > 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.
> For the `-O0` pipeline, we never run any of the normal instruction simplification passes. That's where the `true` case is normally handled. I think it is in the spirit of the `-O0` pipeline to run only one instance of the pass at all, so that would naturally mean a single late folding.  When I look at `CodeGen/AArch64/O0-pipeline.ll`, it can't be run much earlier anyway, so it shouldn't make much of a difference in this case anyway.
Okay. I think the comment can stand as is though, unless you want to explicitly call out the -O0 case.


================
Comment at: lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp:44
+  if (Exp) {
+    II->replaceAllUsesWith(Exp);
+    II->eraseFromParent();
----------------
joerg wrote:
> void wrote:
> > 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.
> Remember, in the -O0 pass chain SimplifyCFG is never run. That seems to be the only pass that changes conditional branches into unconditional branches. We always run DCE, so exploiting that one is fine. In D65280, one option of extending instruction simplify is implemented, but that one certainly needs some thoughts on whether it is the correct way forward.
Right. So I'm not sure about your comment that `replaceAndRecursivelySimplify` not being good enough matters, since -O0 has no guarantees about performance. The only way it could matter is if code that's expected to be dead, and would otherwise cause a compilation error of some sort, is left over.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64963