HomePhabricator

Return "[InstCombine] Simplify compare of Phi with constant inputs against a…

Authored by mkazantsev on Jun 5 2020, 6:44 AM.

Description

Return "[InstCombine] Simplify compare of Phi with constant inputs against a constant"

This reverts commit c4b5a66e44f031eb89c9d6ea32b144f1169bdbae.

Returning along with Clang test fix

Event Timeline

samparker added a subscriber: samparker.EditedJun 8 2020, 7:17 AM

Hi Max,
This has caused quite a lot of changes in our downstream testing, increasing code size and reducing execution speed. Having a brief look at the patch (and none of our codegen changes), I'm a bit dubious about creating i1 phis because the values won't live in normal registers and it's possible that we're having to do something more expensive than a icmp to rematerialise the values. Is this transform supposed to be universally good, or can instcombine be controlled by target hooks?

@samparker revert this patch until concerns are not resolved

@spatel @nikic

This change looks pretty sensible to me, as a general canonicalization. @samparker If your target has unusual i1 representation, shouldn't it be possible to codegen i1 phis by essentially reversing this transformation? That is create iYOUR_PREFERRED_WIDTH phi of 0/1 (or so) and compare it to zero?

The usual rule of thumb here is that if changes to IR canonicalization cause regressions in the backend, it's generally the backends job to do something about it, because the IR could have already come in that form originally.

Can you post PR with example of perf regression?

@nikic did you measure some regressions on your website (x86)?

Hi,

I've been struggling to create a reproducer, though I can see the changes happening. I think my real problem is that simplify-cfg creates selects of i1 after the phi insertions. I will revert my revert and look into solving the problem.