[DAGCombiner] fold (addcarry (xor a, -1), b, c) -> (subcarry b, a, !c) and flip carry.

[deadalnix]

Summary

As per title. DAGCombiner only mathes the special case where b = 0, this patches extends the pattern to match any value of b.

Depends on D57302

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2413 ↗

(On Diff #190083)

NFC - this can be committed straight away?

2423 ↗

(On Diff #190083)

Drop unnecessary getNode() ?

[deadalnix]

Rebase, extract the NFC changes into a NFC patch and remove redundant getNode call.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2711 ↗

(On Diff #207821)

Are there any callers that use Force == false?

[deadalnix]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2711 ↗

(On Diff #207821)

I have to be honest here, that patch is from march and my memory isn't super fresh. Reading the code, this seems wrong and I'm surprised things work at all. When force is set to true, then extractBooleanFlip will ALWAYS flip the boolean, even if that means adding new ops. Clearly, this is not what is intended in the select case for instance. It's somewhat surprising that no test broke. I'll investigate further and see if I can come up with a test case for select.

[deadalnix]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2711 ↗

(On Diff #207821)

After further investigation, it turns out that select use a boolean predicate, so Force cannot have any effect (1 is already recognized as a flip, 0 is a noop and a constant gets optimized away).

[deadalnix]

Add a comment explainign what extractBooleanFlip does.

By default, only flip the boolean if it is cheaper to compute.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2718 ↗

(On Diff #207909)

It's probably better for Force not to have a default value?

[deadalnix]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

2718 ↗

(On Diff #207909)

I don't feel strongly either way so I'll remove the default value.

[deadalnix]

Remove default value for extractBooleanFlip's Force parameter.

[RKSimon]

LGTM - cheers