Page MenuHomePhabricator

[X86] Do not lower scalar sdiv/udiv to a shifts + mul sequence when optimizing for minsize
ClosedPublic

Authored by mkuper on Aug 17 2015, 7:50 AM.

Details

Summary

There are some cases where the mul sequence is smaller, but for the most part, using a div is preferable.

This does not apply to vectors, since x86 doesn't have vector idiv, and a vector mul/shifts sequence ought to be smaller than a scalarized division.
(Of course, this really depends on the type, since we may not have vector muls/shifts either, but I'd rather just keep the existing behavior for the vector case)

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 32295.Aug 17 2015, 7:50 AM
mkuper retitled this revision from to [X86] Do not lower scalar sdiv/udiv to a shifts + mul sequence when optimizing for minsize.
mkuper updated this object.
mkuper added reviewers: hfinkel, spatel, RKSimon.
mkuper added subscribers: llvm-commits, zansari.
spatel edited edge metadata.Aug 17 2015, 10:37 AM

Since we're making these virtual functions, can we have them return 'false' by default and eliminate the member variables (IntDivIsCheap, Pow2SDivIsCheap) in TargetLowering? It doesn't look like anything in tree even overrides these, so changes would be minimal.

Thanks, Sanjay.

I was thinking about preserving the API for out-of-tree targets, but, I agree, it does look somewhat redundant. You're probably right, the state variables can go away.

Also, it doesn't look like there are any in-tree targets that actually have different settings for isPow2SDivCheap() and isIntDivCheap().
Looking at the history, isPow2SDivCheap() was introduced in 2005 for the benefit of PPC (with the comment "This will probably go away in the future."), and PPC was the only in-tree user until 2014, at which point Hal removed that use.

I'll update the patch to remove the Pow2SDiv interface entirely, and remove the state variable for IntDiv.

mkuper updated this revision to Diff 32389.Aug 18 2015, 2:08 AM
mkuper added a reviewer: arsenm.

Updated to refactor the TLI interface as suggested.
I'll split this into two patches when I commit - one for the refactoring, and one for the x86 change.

spatel accepted this revision.Aug 18 2015, 7:43 AM
spatel edited edge metadata.

A couple of nits in the comments. Otherwise, LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
2188 ↗(On Diff #32389)

Missing period.

lib/Target/X86/X86ISelLowering.cpp
26428–26431 ↗(On Diff #32389)

Use "division" in these lines rather than "div"

This revision is now accepted and ready to land.Aug 18 2015, 7:43 AM
This revision was automatically updated to reflect the committed changes.