Page MenuHomePhabricator

[GlobalISel] Add sdiv exact (X, constant) -> mul combine.
ClosedPublic

Authored by aemerson on Jul 25 2022, 2:30 PM.

Details

Summary

This port of the SDAG optimization is only for exact sdiv case.

Diff Detail

Event Timeline

aemerson created this revision.Jul 25 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 2:30 PM
aemerson requested review of this revision.Jul 25 2022, 2:30 PM
arsenm added inline comments.Jul 25 2022, 2:36 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

This greatly depends on the target handling of division and probably should be attached to the tablegen addition

tschuett added inline comments.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5015

Is this cuter as an if-statement to get rid of the (void)?

aemerson added inline comments.Jul 25 2022, 3:18 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

Not sure what you mean by "tablegen addition"?

5015

I don't think so, (void) is the common idiom for silencing unused variable warnings.

arsenm added inline comments.Jul 25 2022, 3:38 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

I mean GICombineRules should have predicate fields, analogous to PatFags

aemerson added inline comments.Jul 25 2022, 4:12 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

Is there a patch to do that already?

arsenm added inline comments.Jul 25 2022, 4:53 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

No, not that I'm aware of

aemerson added inline comments.Jul 25 2022, 9:17 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

Implementing the infrastructure for that seems a separate issue though? This kind of minsize check is mirroring the same check done for SelectionDAG.

foad added inline comments.Jul 26 2022, 2:15 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4963

Just return C && !C->isNullValue()?

5004

Use APInt::multiplicativeInverse instead?

5021

Is this a way of testing whether the divisor is not a splat? It seems a bit hacky. If the divisor is a splat then we could have saved some work above by only computing the multiplicative inverse once.

5025

How does this work? It seems like it would use a scalar shift amount for a vector shift, which is not allowed by MachineVerifier.

arsenm added inline comments.Jul 26 2022, 5:40 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4955–4958

Yes. I didn't notice that check; I should probably look into finding a way to disable it there too

aemerson added inline comments.Jul 26 2022, 7:39 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5021

No, it's just checking if it's a vector type. You're right if it's a splat we can do better here.

5025

As I said this else is actually just handling the scalar case.

aemerson updated this revision to Diff 447927.Jul 26 2022, 9:19 PM

Address comments, add test for non-splat and splat vector cases.

Fix a needed util function not having the right symbol name.

aemerson updated this revision to Diff 447929.Jul 26 2022, 9:20 PM

Add missing change.

foad added inline comments.Jul 27 2022, 1:45 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
5037

Comment is misleading. You're shifting the dividend based on trailing zeros in the divisor, so we don't know that the value is even before shifting, and we don't know the LSB is one after shifting.

aemerson updated this revision to Diff 448081.Jul 27 2022, 9:41 AM

Remove misleading comment.

foad added inline comments.Aug 2 2022, 3:05 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4944

This seems redundant given the call to matchUnaryPredicate below. (I don't think it's significantly cheaper than matchUnaryPredicate is it?)

5004

Ping. Is there a reason not to use it?

5013

Is zero extending really correct here? It feels dubious for a signed division but I'm not sure of the maths at this point.

aemerson added inline comments.Aug 7 2022, 3:10 PM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
4944

Yeah I think you're right.

5004

I did below?

5013

This is also getting beyond the realms on my expertise, but the documentation for it says that the inverse should always fit into the bit width, so extending and truncating shouldn’t affect the result in anyway.

aemerson updated this revision to Diff 450675.Aug 7 2022, 4:00 PM

Remove redundant constant operand check.

Nit: in the first two asserts the explaining string is missing.

Nit: in the first two asserts the explaining string is missing.

Sure I can add one before commit.

foad accepted this revision.Thu, Sep 1, 3:51 AM
This revision is now accepted and ready to land.Thu, Sep 1, 3:51 AM
This revision was automatically updated to reflect the committed changes.