Page MenuHomePhabricator

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

Authored by deadalnix on Mar 11 2019, 7:02 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

deadalnix created this revision.Mar 11 2019, 7:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 7:02 AM
RKSimon added inline comments.Jul 1 2019, 7:17 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2413 ↗(On Diff #190083)

NFC - this can be committed straight away?

2423 ↗(On Diff #190083)

Drop unnecessary getNode() ?

deadalnix updated this revision to Diff 207821.Jul 3 2019, 9:43 AM

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

RKSimon added inline comments.Jul 3 2019, 10:05 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2711 ↗(On Diff #207821)

Are there any callers that use Force == false?

deadalnix marked an inline comment as done.Jul 3 2019, 2:50 PM
deadalnix added inline comments.
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 marked an inline comment as done.Jul 3 2019, 4:01 PM
deadalnix added inline comments.
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 updated this revision to Diff 207909.Jul 3 2019, 4:06 PM

Add a comment explainign what extractBooleanFlip does.

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

RKSimon added inline comments.Jul 8 2019, 4:18 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2718 ↗(On Diff #207909)

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

deadalnix marked an inline comment as done.Jul 11 2019, 4:37 AM
deadalnix added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2718 ↗(On Diff #207909)

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

deadalnix updated this revision to Diff 209182.Jul 11 2019, 5:25 AM

Remove default value for extractBooleanFlip's Force parameter.

RKSimon accepted this revision.Jul 14 2019, 7:51 AM

LGTM - cheers

This revision is now accepted and ready to land.Jul 14 2019, 7:51 AM
This revision was automatically updated to reflect the committed changes.