@andrew.w.kaylor - ping, do you have any comments about the latest revision?

Add the comment about overly conservative legality check, add the test demonstrating it.

Update, I didn't see that there was a request to add a comment. Will do and update the patch,

Address review comments.

In D31182#706365, @spatel wrote:

Before getting to any details about the patch, we need to address the question raised in PR27036: why are we doing this transform in InstCombine at all? The assumption is that an integer add is more canonical and/or cheaper than an FP add. Is that universally true?

It's difficult to tell. Even if FP addition is cheaper the second combine also saves us one sitofp conversion. It should be also taken into account.

It's worth noting for others that the motivation for the change is a bug we found in JumpThreading LVI cache invalidation. The bug is easily triggered with our downstream changes, but is difficult to trigger it upstream. We need the printer pass to write an isolated upstream test case for this problem.

Add an assert as suggested.

• Land tests separately and rebase the patch against it.
• Remove addrspace(1) from the test cases.
• Change CHECK pattern matching in fp16-promote.ll test_extractelement slightly.

Fix the type CHECK64 -> CHECK-ARMv6 in ARM load-combine.ll test case.

The test cases have been landed separately (rL294185), rebase the patch.

Address Sanjoy's comments: add new pass manager support, fix nits here and there.
Add an early exit if the module doesn't use guards.

@chandlerc since none of your comments express explicit objections or concerns and the patch has already been accepted I'm going to go ahead and land it tomorrow. Let me know if I misinterpreted your comments and you do want some changes.

Address review comments:

• Add recursion depth limit
• Add comments about combine before legalize

Address the comments, add a comment that calculateByteProvider recurses over trees, not DAGs.

Currently update_llc_test_checks.py supports arm-eabi target only. I left ARM test cases with manually written checks for now.

Address the comments.

General practice in LLVM is to make small isolated changes. In this review you seem to have at least two unrelated changes:

1. Fixing the segfault,
2. Fixing the output issue.

Both changes are fine on their own, but when they are mixed in one commit it's more difficult to review and reason about. Given BrainF is an example it's not a big deal, but in general it's considered to be a bad practice.