This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Adding Z / (1.0 / Y) => (Y * Z)
ClosedPublic

Authored by raghesh on Jan 7 2020, 12:38 AM.

Details

Summary

This is a special case of Z / (X / Y) => (Y * Z) / X, with X = 1.0. The m_OneUse check is avoided because even in the case of the multiple uses for 1.0/Y, the number of instructions remain the same and a division is replaced by a multiplication.

The following testcase is updated.

test/Transforms/InstCombine/fdiv.ll

Diff Detail

Event Timeline

raghesh created this revision.Jan 7 2020, 12:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2020, 12:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
spatel added inline comments.Jan 7 2020, 7:17 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
1249

I don't think these constant checks are necessary. (I'm not sure why they are part of the existing code either - maybe some corner case with denorm values?)

If Y is a constant, 1.0/C will get constant folded and Z/C' will eventually get folded too, so this should just get to canonical form quicker. The 'Z' is constant pattern is similar.

FDiv with constant operand are generally handled within:
foldFDivConstantDivisor()
foldFDivConstantDividend()

llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll
15–21
  1. Add a test within test/Transforms/InstCombine/fdiv.ll instead of creating a new file.
  2. Create minimal test(s) to show the transform; in particular, this transform does not require any of alloca, load, store, so the test shouldn't have those.
  3. Use this script to auto-generate the CHECK lines:

utils/update_test_checks.py

  1. I prefer that you add the test *without* this code patch showing the current IR produced as a preliminary patch. That way, we will see the diff from the new transform when this patch is committed.
  2. If you need help committing/pushing the patches, let me know.
raghesh added inline comments.Jan 7 2020, 10:55 PM
llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll
15–21

Thanks for the commets. I will address these.

However, point number 4 is not clear to me. Could you please explain it?

Yes. As I don't have commit access, I will need your help in committing it.

craig.topper added inline comments.Jan 7 2020, 11:13 PM
llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll
15–21

For point 4. Create a new review containing just the new test case with no other changes. This will show what the current behavior is. Then rebase this review to be on top of that so that this review just shows how the test changes with the transformation.

raghesh marked an inline comment as done.Jan 8 2020, 1:42 AM
raghesh added inline comments.
llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll
15–21

Test case added with a new reivew https://reviews.llvm.org/D72388

spatel added inline comments.Jan 8 2020, 7:42 AM
llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll
15–21

Thanks - that was pushed here:
rG5dfd52398f5c

You can now rebase/update this patch.

raghesh updated this revision to Diff 237069.Jan 9 2020, 6:54 AM
raghesh edited the summary of this revision. (Show Details)
raghesh removed a reviewer: llvm-commits.

This revision is updated with the expected change in the testcase test/Transforms/InstCombine/fdiv-to-fmul-opt.ll. Other comments are addressed too.

spatel accepted this revision.Jan 9 2020, 7:13 AM

LGTM

This revision is now accepted and ready to land.Jan 9 2020, 7:13 AM
This revision was automatically updated to reflect the committed changes.