Page MenuHomePhabricator

[InstCombine] Missed optimization in math expression: squashing sqrt functions
ClosedPublic

Authored by Quolyk on Dec 16 2017, 7:41 AM.

Diff Detail

Event Timeline

Quolyk created this revision.Dec 16 2017, 7:41 AM
spatel added inline comments.Dec 16 2017, 8:01 AM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
626

Note that we have a fast-math-flag specifically for reassociation now (as well as a FMF for approximation of math functions). I'm not sure what combination would be needed for this transform. It's independent of this patch, but something to think about.

731

What if sqrt(a) or sqrt(b) has another use besides the multiply? I don't think we want to do the transform in that case. Either way, please add a test for that kind of example.

733–739

You can use:

match(Op0, m_Intrinsic<Intrinsic::sqrt>( ...

to simplify this code.

test/Transforms/InstCombine/fmul.ll
185–199

Please use the script at utils/update_test_checks.py to auto-generate the checks for any tests you add. The checks will be more exact and have regex for the variable names, so they'll be more robust.

davide requested changes to this revision.Dec 16 2017, 9:36 AM
davide added a subscriber: davide.
davide added inline comments.
test/Transforms/InstCombine/fmul.ll
195–199

Please add a negative test showing that this combine doesn't fire unless the conditions are met (allows reassociation/fast-math, etc..)

This revision now requires changes to proceed.Dec 16 2017, 9:36 AM
Quolyk retitled this revision from [InstCombine] Missed optimization in math expression: squashing sqrt functions to [WIP][InstCombine] Missed optimization in math expression: squashing sqrt functions.Dec 18 2017, 1:17 AM
hfinkel added inline comments.Dec 19 2017, 8:13 PM
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
731

I agree, we should have ->hasOneUse() checks here.

740

Needs

BuilderTy::FastMathFlagGuard Guard(Builder);

above this.

Quolyk updated this revision to Diff 127837.Dec 21 2017, 12:38 AM
Quolyk retitled this revision from [WIP][InstCombine] Missed optimization in math expression: squashing sqrt functions to [InstCombine] Missed optimization in math expression: squashing sqrt functions.
Quolyk marked 4 inline comments as done.
spatel accepted this revision.Dec 21 2017, 4:30 AM

LGTM

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
737

Just a few small bits here:

  1. You don’t have to initialize to nullptr; the captured variables aren’t touched if the match fails.
  2. There is a matcher for m_OneUse (though it may not save any space here).
  3. I prefer to keep the names in the code comments and the actual code variables the same, so Opnd0 would just be ‘A’ etc.

@davide please accept the revision, if ok. I'll commit.

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
737
  1. I know, Xcode showed warning at that line, so I decided to make it that way.
  2. That is useful, didn't know that.
  3. I'll take into account, thank you.
davide accepted this revision.Dec 22 2017, 11:14 AM
This revision is now accepted and ready to land.Dec 22 2017, 11:14 AM

This isn't committed yet. Do you have access?

This isn't committed yet. Do you have access?

I have been granted a few days ago, just need to figure out how to make it.

Quolyk edited the summary of this revision. (Show Details)Jan 1 2018, 9:57 PM
Quolyk closed this revision.Jan 1 2018, 9:59 PM