Page MenuHomePhabricator

Add a pass to lower is.constant intrinsics
Needs ReviewPublic

Authored by void on Jul 18 2019, 5:07 PM.

Details

Summary

THIS IS A WORK IN PROGRESS. NOT FOR SUBMISSION.

This pass lowers the is.constant intrinsics into 'true' or 'false'. We
want to convert to 'true' early in the pipeline and 'false' late in the
pipeline. In other words, if right before code generation we cannot
definitively prove that the argument is constant, then we will lower to
'false'.

Diff Detail

Event Timeline

void created this revision.Jul 18 2019, 5:07 PM
void updated this revision to Diff 210707.Jul 18 2019, 5:13 PM

Remove other commit.

joerg added inline comments.Jul 19 2019, 5:37 AM
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
10

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.

45

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?

void marked 2 inline comments as done.Jul 29 2019, 1:47 PM
void added inline comments.
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
10

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.

45

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.

joerg added inline comments.Jul 29 2019, 2:44 PM
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
10

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.

45

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.

void marked 2 inline comments as done.Jul 29 2019, 2:55 PM
void added inline comments.
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
10

Okay. I think the comment can stand as is though, unless you want to explicitly call out the -O0 case.

45

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.

joerg added inline comments.Jul 29 2019, 3:08 PM
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
45

Exactly, the inline asm statements that triggered the original case. If they are left around, they can trigger the I-want-immediate-operands checks.

void marked an inline comment as done.Jul 29 2019, 3:35 PM
void added inline comments.
lib/Transforms/Scalar/LowerIsConstantIntrinsic.cpp
45

God bless inline asm. :-)