This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Refactor DAGCombiner::ReassociateOps
ClosedPublic

Authored by bjope on Apr 26 2019, 9:52 AM.

Details

Summary

Extract the logic for doing reassociations
from DAGCombiner::reassociateOps into a helper
function DAGCombiner::reassociateOpsCommutative,
and use that helper to trigger reassociation
on the original operand order, or the commuted
operand order.

Codegen is not identical since the operand order will
be different when doing the reassociations for the
commuted case. That causes some unfortunate churn in
some test cases. Apart from that this should be NFC.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Apr 26 2019, 9:52 AM
bjope added inline comments.Apr 26 2019, 10:03 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1022 ↗(On Diff #196872)

An alternative to avoid churn in test cases is to provide some bool indicating that we are handling the commuted scenario, and then swap the operand order here (and also on line 1028). I think that would result in the same operand ordering as in the past (but since the opcodes handled here are commutative it shouldn't matter for correctness).

I don't mind the test churn, but is there a motivating test that shows that we missed an optimization? If not (and this patch is virtually NFC), then let's include that in the description/commit message.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
461 ↗(On Diff #196872)

ReassociateOpsCommutative -> reassociateOpsCommutative (lowerCamelCase)

Please fix the existing "ReassociateOps" as part of this patch or as a preliminary cleanup too. If we keep using the wrong formatting based on existing code, we'll never converge on the right formatting. :)

1005 ↗(On Diff #196872)

Trying -> Try

1039–1041 ↗(On Diff #196872)

I'm not sure if we have a utility function to enforce this, but it would be nice to assert that Opc is actually a commutative opcode.

bjope updated this revision to Diff 197036.Apr 28 2019, 12:46 PM

Address review comments from @spatel (thanks!).

bjope edited the summary of this revision. (Show Details)Apr 28 2019, 12:47 PM
bjope edited the summary of this revision. (Show Details)
bjope marked 2 inline comments as done.
spatel accepted this revision.Apr 29 2019, 7:56 AM

LGTM

This revision is now accepted and ready to land.Apr 29 2019, 7:56 AM
This revision was automatically updated to reflect the committed changes.