Page MenuHomePhabricator

[InstCombine] Relaxed constraints of uses for exp(X) * exp(Y) -> exp(X + Y) and exp2(X) * exp2(Y) -> exp2(X + Y)
ClosedPublic

Authored by vdsered on May 18 2021, 9:51 AM.

Details

Summary

InstCombine didn't perform the transformations when fmul's operands were the same instruction because it required to have one use for each of them which is false in the case. This patch fixes this + adds tests for them and introduces a new function isOnlyUserOfAnyOperand to check these cases in a single place.

This patch is a result of discussion in https://reviews.llvm.org/D102574.

Diff Detail

Unit TestsFailed

TimeTest
240 msx64 windows > LLVM.DebugInfo/X86::basic-block-sections-debug-loc-const-value-1.ll
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\llc.exe C:\ws\w5\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\basic-block-sections-debug-loc-const-value-1.ll --dwarf-version=4 --basic-block-sections=none -filetype=obj -o C:\ws\w5\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\basic-block-sections-debug-loc-const-value-1.ll.tmp
240 msx64 windows > LLVM.DebugInfo/X86::basic-block-sections-debug-loc-const-value-2.ll
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\llc.exe C:\ws\w5\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\basic-block-sections-debug-loc-const-value-2.ll --dwarf-version=4 --basic-block-sections=none -filetype=obj -o C:\ws\w5\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\basic-block-sections-debug-loc-const-value-2.ll.tmp
260 msx64 windows > LLVM.DebugInfo/X86::basic-block-sections-debug-loc.ll
Script: -- : 'RUN: at line 5'; c:\ws\w5\llvm-project\premerge-checks\build\bin\llc.exe C:\ws\w5\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\basic-block-sections-debug-loc.ll --dwarf-version=4 --basic-block-sections=none -filetype=obj -o C:\ws\w5\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\basic-block-sections-debug-loc.ll.tmp
210 msx64 windows > LLVM.DebugInfo/X86::basic-block-sections-debug-loclist-4.ll
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\llc.exe C:\ws\w5\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\basic-block-sections-debug-loclist-4.ll --dwarf-version=4 --basic-block-sections=none -filetype=obj -o C:\ws\w5\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\basic-block-sections-debug-loclist-4.ll.tmp
220 msx64 windows > LLVM.DebugInfo/X86::basic-block-sections-debug-loclist-5.ll
Script: -- : 'RUN: at line 1'; c:\ws\w5\llvm-project\premerge-checks\build\bin\llc.exe C:\ws\w5\llvm-project\premerge-checks\llvm\test\DebugInfo\X86\basic-block-sections-debug-loclist-5.ll --dwarf-version=4 --basic-block-sections=none -filetype=obj -o C:\ws\w5\llvm-project\premerge-checks\build\test\DebugInfo\X86\Output\basic-block-sections-debug-loclist-5.ll.tmp

Event Timeline

vdsered created this revision.May 18 2021, 9:51 AM
vdsered requested review of this revision.May 18 2021, 9:51 AM
efriedma added inline comments.
llvm/include/llvm/Transforms/Utils/Local.h
470 ↗(On Diff #346197)

Don't expose isRestrictiveUseConstraintMet in the header if you don't plan to use it anywhere.

474 ↗(On Diff #346197)

I don't like the name isRelaxedUseConstraintMet; doesn't provide any indication what it means at first glance. Maybe isOnlyUserOfAnyOperand?

llvm/lib/Transforms/Utils/Local.cpp
3311 ↗(On Diff #346197)

llvm::is_splat?

vdsered updated this revision to Diff 346436.May 19 2021, 6:40 AM
vdsered edited the summary of this revision. (Show Details)
vdsered added a reviewer: efriedma.

Updating diff

  1. Fixed warnings of clang-tidy
  2. Refactored implementation for this patch
vdsered added inline comments.May 19 2021, 6:50 AM
llvm/include/llvm/Transforms/Utils/Local.h
470 ↗(On Diff #346197)

I saw how a check like Op0->hasOneUse() || Op1->hasOneUse() is heavily used as a precondition for many patterns and I thought of making it as a helper here because isOnlyUserOfAnyOperand uses it too. However, there are also places where one writes something like match(Op0, m_OneUse(...)) and it can probably be a little bit better in terms of readability. So, I'm not sure if we actually need this function. Removed it here as it is not that important for the goal of the patch.

474 ↗(On Diff #346197)

Your suggestion is a good alternative to the previous name. Renamed it.

llvm/lib/Transforms/Utils/Local.cpp
3311 ↗(On Diff #346197)

Fixed.

vdsered updated this revision to Diff 346466.May 19 2021, 8:07 AM
vdsered updated this revision to Diff 346630.May 19 2021, 9:56 PM
spatel added inline comments.May 23 2021, 7:30 AM
llvm/lib/Transforms/Utils/Local.cpp
3309–3310 ↗(On Diff #346630)

Use llvm::any_of() to shorten, and implicit convert operand list to Value?

return any_of(I.operands(), [](Value *V) { return V->hasOneUse();}) || ...
3312 ↗(On Diff #346630)

I->operands() ?

vdsered updated this revision to Diff 347353.May 24 2021, 5:12 AM

Replaced std::any_of with llvm::any_of and used I->operands() instead of making range directly via begin/end

vdsered added inline comments.May 24 2021, 5:14 AM
llvm/lib/Transforms/Utils/Local.cpp
3309–3310 ↗(On Diff #346630)

Fixed.

3312 ↗(On Diff #346630)

Fixed.

vdsered updated this revision to Diff 347359.May 24 2021, 5:49 AM

Removed irrelevant comments

spatel added inline comments.May 24 2021, 5:51 AM
llvm/lib/Transforms/Utils/Local.cpp
3305 ↗(On Diff #347353)

This seems inverted. How about creating an inspection function within the Instruction class itself?

bool Instruction::isOnlyUserOfAnyOperand() const;

I"m not sure i like isOnlyUserOfAnyOperand() direction,
even though i've encountered this problem before.

I would recommend to simply use hasOneUser() in place of hasOneUse()?

As I understand it

if (match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) &&
        match(Op1, m_Intrinsic<Intrinsic::exp2>(m_Value(Y))) &&
        isOnlyUserOfAnyOperand(&I))

will be

if (match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) &&
        match(Op1, m_Intrinsic<Intrinsic::exp2>(m_Value(Y))) &&
        (X->hasOneUser() || Y -> hasOneUser()))

The difference between them is that isOnlyUserOfAnyOperand depends on how many operands an instruction has (a known count, we use it for mul now where it is 2). On the other, hasOneUser depends on the total number of uses of each operand (can be an arbitrary number). I'm not sure, but there can theoretically be situations when a number of uses is more than a number of ops, right?

@lebedev.ri, we could implement isOnlyUserOfAnyOperand like this

bool llvm::isOnlyUserOfAnyOperand(Instruction *I) {
    return llvm::any_of(I->operands(), [](Value *V) { return V->hasOneUser(); })
}

This solution would be good too especially because it is easier to understand, but I'd leave it as-is without hasOneUser in case there are no any performance/logical issues with it.

vdsered added inline comments.May 24 2021, 7:04 AM
llvm/lib/Transforms/Utils/Local.cpp
3305 ↗(On Diff #347353)

I'm a little bit confused where to place such a function because here we have, for example, isInstructionTriviallyDead-like functions, but we have more those within Instruction class. I move it to the class.

vdsered updated this revision to Diff 347395.May 24 2021, 7:47 AM

Moved isOnlyUserOfAnyOperand to Instruction class

bool llvm::isOnlyUserOfAnyOperand(Instruction *I) {
    return llvm::any_of(I->operands(), [](Value *V) { return V->hasOneUser(); })
}

This solution would be good too especially because it is easier to understand, but I'd leave it as-is without hasOneUser in case there are no any performance/logical issues with it.

I didn't understand the comment. That implementation definitely looks nicer than what we have in the current patch (nit: no need to explicitly use llvm:: namespace prefix). Is there a reason not to do that (or the alternate suggestion of changing the caller code only with hasOneUser()? I slightly prefer adding this inspection method because it will make the caller code more obviously different than the usual hasOneUse() code pattern, but either way seems fine to me.

vdsered updated this revision to Diff 347692.May 25 2021, 8:44 AM

isOnlyUserOfAnyOperand uses hasOneUser to figure out if at least one of ops has only one user

I pushed some baseline tests here:
01120fe5b398

Please rebase/update the patch after that (manually remove TODO comments in the test file).

Other than that, this LGTM now, but let's see if there is agreement on that with other reviewers.

vdsered updated this revision to Diff 348204.May 27 2021, 3:36 AM

Rebasing + updated tests with multiple uses in fmul

vdsered updated this revision to Diff 348217.May 27 2021, 4:00 AM
vdsered updated this revision to Diff 348437.May 27 2021, 8:11 PM

Waiting for approval of other reviewers

This revision is now accepted and ready to land.May 28 2021, 12:40 PM
vdsered added a comment.EditedJun 1 2021, 4:04 AM

I ask someone to commit the patch if there are no any barriers left for that (Roman's agreement?). I don't have commit access.