Page MenuHomePhabricator

[DAGCOmbine] Ensure that (brcond (setcc ...)) is handled in a canonical manner.

Authored by deadalnix on Dec 14 2017, 6:10 AM.



There are transformation that change setcc into other constructs, and transform that try to reconstruct a setcc from the brcond condition. Depending on what order these transform are done, the end result differs.

Most of the time, it is preferable to get a setcc as a brcond argument (and this is why brcond try to recreate the setcc in the first place) so we ensure this is done every time by also doing it at the setcc level when the only user is a brcond.

Diff Detail


Event Timeline

deadalnix created this revision.Dec 14 2017, 6:10 AM
niravd added inline comments.Dec 15 2017, 8:56 AM
7014 ↗(On Diff #126939)

Should this be "return SDValue();"?

15 ↗(On Diff #126939)

It looks like all the bittest patterns are no longer being matched because of the DAG change.

deadalnix added inline comments.Jan 1 2018, 5:10 PM
7014 ↗(On Diff #126939)

I think this is correct. If no combine is found, then SDValue is returned (line 6999) and if there is, the code tries to rebuild a setcc if that is a preferred form and if not, then we just have the combine done :)

15 ↗(On Diff #126939)

Could you point me to the code that generate these patterns so it can be fixed ?

More generally, this match is likely broken half the time in the wild right now, because the form generated depends on the order in which nodes are processed in the DAGCombiner.

niravd added inline comments.Jan 8 2018, 9:35 AM
15 ↗(On Diff #126939)

It's likely a missed case in the BT instruction generation in X86ISelLowering.cpp

Incidentally D41293 has a somewhat related change that improves DAG node fusion in instruction selection. It doesn't resolve this issue (though it exposes a similar missing case in unfoldMemoryOperand) but using both of these patches exposes a matching issue in a Hexagon test which you may be interested in.

Rebase on top of D42615, which ensure there are no more regression for pattern involving test/bt.

@niravd After proposing D42615 , it seems like using test instead of bt is the right thing to do as it has higher throughput - unless more work is required to materialize the constant. There is a bug in the materialization of test that cause it to sometime create needless copy. This needs to be fixed, regardless of what this diff does. I think we should proceed with this one.

Okay. This looks good. Just one more quick sanity check: Is this patch rebased off of D42615 or without it? I just want to makre sure that we've not missed any test changes.

@niravd It is rebased on top of D42615 . Alternatively, it can be rebased on top of D42646 with identical results as, as per discussion with @craig.topper , the test instruction has higher throughput than bt so maybe that's a preferable solution. Either way, the high register trick problem has been identified and there are several possible solutions.

niravd accepted this revision.Jan 29 2018, 12:45 PM


This revision is now accepted and ready to land.Jan 29 2018, 12:45 PM
deadalnix updated this revision to Diff 135622.Feb 23 2018, 3:46 AM

Add bugfix for Hexagon and rebase

This revision was automatically updated to reflect the committed changes.