This is an archive of the discontinued LLVM Phabricator instance.

[X86][Codegen] Shift amount mod: sh? i64 x, (32-y) --> sh? i64 x, -(y+32)
ClosedPublic

Authored by lebedev.ri on May 5 2021, 1:56 PM.

Details

Summary

I've seen this in the wild when looking into a +5% runtime regression.
I was hoping that this would fix it, but it doesn't.

I'm not certain whether this is always beneficial?
But i'm actually mainly interested in the case where we already compute (y+32) (see last test),
so if we aren't okay with this in general, i can try restricting it to getNodeIfExists().

https://alive2.llvm.org/ce/z/ZCzJio

Diff Detail

Event Timeline

lebedev.ri created this revision.May 5 2021, 1:56 PM
lebedev.ri requested review of this revision.May 5 2021, 1:56 PM
lebedev.ri edited the summary of this revision. (Show Details)May 5 2021, 1:59 PM

I think this does make sense, the increased use of basic 2op LEAs shouldn't be a perf issue, and we should reduce register pressure.

Any other comments?

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
3859

Please can you rebase this? I've tried to cleanup some of the repeated casts

lebedev.ri added inline comments.May 11 2021, 6:25 AM
llvm/test/CodeGen/X86/64-bit-shift-by-32-minus-y.ll
293–300

To reiterate, i'm most interested in this case, where we already computed the x+32, so we get that for free.

lebedev.ri marked an inline comment as done.

@RKSimon thank you for taking a look!
Rebased.

This would regress the case if the 32-x has another use (add test)?

define i64 @sub_cse(i64 %val, i64 %shamt, i64*%dst) nounwind {
  %negshamt = sub i64 32, %shamt
  store i64 %negshamt, i64* %dst
  %shifted = shl i64 %val, %negshamt
  ret i64 %shifted
}

Adding testcase from @spatel.

Don't transform if the old sub has other uses.

spatel accepted this revision.May 11 2021, 9:24 AM

hasOneUse() protects against regression. The other cases look neutral or better, so LGTM.

This revision is now accepted and ready to land.May 11 2021, 9:24 AM

hasOneUse() protects against regression. The other cases look neutral or better, so LGTM.

Thank you for the review!

@lebedev.ri https://bugs.llvm.org/show_bug.cgi?id=50431 appears to be due to this - please can you take a look at @craig.topper's proposed fix?