This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Attempt to replace a mul node with an existing umul_lohi/smul_lohi node (PR59217)
ClosedPublic

Authored by RKSimon on Nov 28 2022, 4:17 AM.

Details

Summary

As discussed on Issue #59217, under certain circumstances the DAG can generate duplicate MUL and MUL_LOHI nodes, often during MULO legalization.

This patch attempts to replace MUL nodes with additional uses of the LO result from the MUL_LOHI node.

Diff Detail

Event Timeline

RKSimon created this revision.Nov 28 2022, 4:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 4:17 AM
RKSimon requested review of this revision.Nov 28 2022, 4:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 4:17 AM
foad added a comment.Nov 28 2022, 5:07 AM

AMDGPU changes LGTM!

RKSimon retitled this revision from [DAG] Attempt to replace a mul nodes with an existing umul_lohi/smul_lohi node (PR59217) to [DAG] Attempt to replace a mul node with an existing umul_lohi/smul_lohi node (PR59217).Nov 28 2022, 8:26 AM
deadalnix accepted this revision.Nov 28 2022, 9:31 AM

The change that are relatively small all come either better or equivalent. While it's hard to evaluate the giant multiplications ones, this definitively looks like a win.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4042

As you noted, the code is not the most elegant, but it gets the job done. I'm not sure there is a better way to write this at the moment.

llvm/test/CodeGen/X86/smul-with-overflow.ll
187

I have no idea how to evaluate if this is better or worse.

This revision is now accepted and ready to land.Nov 28 2022, 9:31 AM
RKSimon added inline comments.Nov 28 2022, 10:03 AM
llvm/test/CodeGen/X86/smul-with-overflow.ll
187

We end up spilling/reloading the (now resued) mul_lohi results instead of recomputing - I think this is the better approach for the DAG to take.

If we want to look at instruction rematerialization then that needs to be done in a later pass which can better compute the cost/benefit.

deadalnix added inline comments.Nov 28 2022, 10:30 AM
llvm/test/CodeGen/X86/smul-with-overflow.ll
187

Agreed.

This revision was landed with ongoing or failed builds.Nov 29 2022, 4:51 AM
This revision was automatically updated to reflect the committed changes.