This is an archive of the discontinued LLVM Phabricator instance.

Don't leave unused divs/rems sitting around in BypassSlowDivision.
ClosedPublic

Authored by jlebar on Oct 28 2016, 10:52 AM.

Details

Summary

This "pass" eagerly creates div and rem instructions even when only one
is needed -- it relies on a later pass (machine DCE?) to clean them up.

This is problematic not just from a cleanliness perspective (this pass
is running during CodeGenPrepare, so should leave the IR in a better
state), but it also creates a problem for instruction selection. If we
always have a div+rem, isel will always select a divrem instruction (if
possible), even when a single div or rem would do.

Specifically, in NVPTX, we want to compute rem from the output of div,
if available. But if a div is not available, we want to leave the rem
alone. This transformation is overeager if div is always available.

Because this code runs as part of CodeGenPrepare, it's nontrivial to
write a test for this change. But this will effectively be tested by
a later patch which adds the aforementioned change to NVPTX isel.

Event Timeline

jlebar updated this revision to Diff 76217.Oct 28 2016, 10:52 AM
jlebar retitled this revision from to Don't leave unused divs/rems sitting around in BypassSlowDivision..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added a subscriber: llvm-commits.
tra added inline comments.Oct 28 2016, 11:13 AM
llvm/lib/Transforms/Utils/BypassSlowDivision.cpp
251

Should it be "erase any unused divs/rems..."?

260

What's supposed to happen with operands that are not instructions?
Destruction of unused subgraph sounds like a common operation, perhaps we already have a function to do it?

jlebar updated this revision to Diff 76243.Oct 28 2016, 2:00 PM
jlebar marked an inline comment as done.

Use helper function to delete dead code, and add an IR test.

jlebar marked an inline comment as done.Oct 28 2016, 2:00 PM
jlebar added inline comments.
llvm/lib/Transforms/Utils/BypassSlowDivision.cpp
251

Indeed, thanks!

260

What's supposed to happen with operands that are not instructions?

We don't need to delete them if they're constants.

Destruction of unused subgraph sounds like a common operation, perhaps we already have a function to do it?

Aha, found it. Thanks. Also, this fixes a bug -- we weren't trying hard enough to delete trivially-dead things here. mkuper pointed me in the right direction and I now have a test.

tra accepted this revision.Oct 28 2016, 2:21 PM
tra edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 28 2016, 2:21 PM
This revision was automatically updated to reflect the committed changes.
jlebar marked an inline comment as done.