Page MenuHomePhabricator

[DAGCombine] Do not fold SRA/SRL of MUL into MULH when MUL's LSB are used, and MUL_LOHI is available

Authored by jmmartinez on Sep 13 2022, 4:11 AM.



Folding into a sra(mul) / srl(mul) into a mulh introduces an extra multiplication to compute the high half of the multiplication,
while it is more profitable to compute the high and lower halfs with a single mul_lohi.

Diff Detail

Unit TestsFailed

60,040 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld

Event Timeline

jmmartinez created this revision.Sep 13 2022, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2022, 4:11 AM
jmmartinez requested review of this revision.Sep 13 2022, 4:11 AM
arsenm added inline comments.Sep 15 2022, 6:51 AM

Probably should use !hasOneUse instead of computing use_size

1 ↗(On Diff #459710)

Test name is too general for what this is testing

3 ↗(On Diff #459710)

This isn't a kernel, rename function

5–7 ↗(On Diff #459710)

Might as well generate these checks

8–9 ↗(On Diff #459710)

Should use named values in test

16 ↗(On Diff #459710)

Can you also add a case with a type with illegal mul_lohi, a vector case, and also a shift with a non-constant case, and an off by one shift amount?

  • Use !hasOneUse instead of use_size
  • renamed test from mul_lohi.ll to dagcomb-mullohi.ll
  • named the instructions in the test using the instnamer
  • generated test assertions using (I wasn't familiar with it, I'm glad to know it now :) )
  • added test-cases for
    • unsigned (already there)
    • signed
    • type for which mul_lohi is not legal
    • vector type
    • non-const shift
    • shift to the right by one more than the limit (so a v_mul_hi is used)
    • shift to the right by one less than the limit (so a v_mad is used)
jmmartinez marked 6 inline comments as done.Sep 16 2022, 4:42 AM

Thanks for the review arsenm. I took into account the reviews in the update of the patch.

I'm not sure if there is a naming convention for the tests in this part: I renamed into dagcomb-mullohi.ll.

arsenm accepted this revision.Sep 16 2022, 6:01 AM
This revision is now accepted and ready to land.Sep 16 2022, 6:01 AM
This revision was landed with ongoing or failed builds.Sep 16 2022, 8:49 AM
This revision was automatically updated to reflect the committed changes.
RKSimon added a subscriber: RKSimon.Dec 7 2022, 8:13 AM
RKSimon added inline comments.

@jmmartinez I'm getting static analysis warnings that this is always true - should this be:

if (U->getOpcode() != ISD::SRL && U->getOpcode() != ISD::SRA) {
jmmartinez added inline comments.Dec 7 2022, 8:20 AM

Well spotted! I'll submit a fix for it. BTW what are you using for static analysis?

RKSimon added inline comments.Dec 7 2022, 8:22 AM

Cheers - VisualAssist 'code inspection' caught this one

RKSimon added inline comments.Dec 7 2022, 8:25 AM

Sorry, amazingly its Intellisense that caught it - they get listed in the same window