This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Add simple folds for SMULFIX/UMULFIX/SMULFIXSAT
ClosedPublic

Authored by bjope on Aug 10 2019, 5:33 AM.

Details

Summary

Add the following DAGCombiner folds for mulfix being
one of SMULFIX/UMULFIX/SMULFIXSAT:

(mulfix x, undef, scale) -> 0
(mulfix x, 0, scale) -> 0

Also added canonicalization of constants to RHS.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.Aug 10 2019, 5:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2019, 5:33 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bjope added inline comments.Aug 10 2019, 5:55 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3468 ↗(On Diff #214521)

typo: "on" -> "of"

bjope updated this revision to Diff 214563.Aug 11 2019, 12:34 PM

Corrected a code comment typo.

bjope marked an inline comment as done.Aug 11 2019, 12:34 PM

Please can you commit mulfix_combine.ll to trunk with current (trunk) codegen, then rebase this patch so that it shows the diff

bjope updated this revision to Diff 215450.Aug 15 2019, 11:42 AM

Update mulfix_combine.ll test case to show diffs compared to trunk.

(Haven't pushed the test case to trunk yet. If that is perferred,
or if you want me to upload it to phab as a parent, I can do it.
Otherwise I'll just land this as two separate commit if this
patch gets accepted.)

It seems like widening vector legalization for X86 can introduce fixed point multiplication of undef values. So that is one way that such operations could appear during ISel.

Multiplication with zero is probably more unlikely, and could potentially be handled by InstCombine. But I do not think it would hurt to do such folds in DAGCombiner.

bjope added inline comments.Aug 15 2019, 11:47 AM
llvm/test/CodeGen/X86/mulfix_combine.ll
16 ↗(On Diff #215450)

X86 backend seem to introduce taget specific DAG nodes for shifts early (without simplifying shift of zero)?
So this might display a more general problem in the X86 backend.

RKSimon accepted this revision.Aug 16 2019, 3:36 AM

LGTM - do we have constant folding coverage btw?

This revision is now accepted and ready to land.Aug 16 2019, 3:36 AM
bjope added a comment.Aug 16 2019, 4:41 AM

LGTM - do we have constant folding coverage btw?

Thanks!

ConstantFolding is handling smul_fix and smul_fix_sat on IR level. But I do not think we do much folds for umul_fix yet (umul_fix_sat has not yet landed - https://reviews.llvm.org/D57836). To complete the work with Embedded-C support in Clang umul_fix_sat will be needed (unfortunately the embedded-C additions has stalled a bit - I'll see if I can find time to finalize D57836 to get things going again - and then I think the next step would be to complete the fixed point multiplication support in clang).

Constant folding in DAG combiner is probably easy to implement as well. But I haven't seen any need for that yet.

This revision was automatically updated to reflect the committed changes.