Page MenuHomePhabricator

[GlobalISel] Start using vectors in GISelKnownBits
ClosedPublic

Authored by Petar.Avramovic on Feb 5 2021, 4:56 AM.

Details

Summary

For vectors we consider a bit as known if it is the same for all demanded
vector elements (all elements by default). KnownBits BitWidth for vector
type is size of vector element. Add support for G_BUILD_VECTOR.
This allows combines of urem_pow2_to_mask in pre-legalizer combiner.

Diff Detail

Event Timeline

Petar.Avramovic created this revision.Feb 5 2021, 4:56 AM
Petar.Avramovic requested review of this revision.Feb 5 2021, 4:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 4:56 AM

"we consider bit as knows if it same for all" -> "we consider a bit as known if it is the same for all"

Looks pretty good to me, but adding some more globalisel folks in case they have comments.

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
171

Don't pass in DemandedElts here.

Petar.Avramovic edited the summary of this revision. (Show Details)Feb 5 2021, 5:30 AM
Petar.Avramovic added inline comments.Feb 5 2021, 5:46 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
171

We don't have such computeKnownBitsImpl in global-isel. Sdag computeKnownBits(SrcOp, Depth + 1) will call computeKnownBits(Op, 'allones', Depth).

foad added inline comments.Feb 5 2021, 5:50 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
171

OK, well I guess it's harmless, since the operand will never have vector type, so I guess it will be ignored. I just found it a bit confusing.

arsenm added a comment.Feb 5 2021, 5:59 AM

Some tests for the non-splat case would be nice

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
164

As a follow up should also try to handle G_BUILD_VECTOR_TRUNC

Adding non-splat test.
isKnownToBeAPowerOfTwo ends up checking known bits for build_vector and fails for non-splat values.

arsenm accepted this revision.Feb 5 2021, 7:55 AM
This revision is now accepted and ready to land.Feb 5 2021, 7:55 AM
aditya_nandakumar accepted this revision.Feb 5 2021, 9:18 AM

Can you add a case to unittests/CodeGen/GlobalISel/KnownBitsTest.cpp as well?

Other than that, this LGTM.

I think lifting the if (DstTy.isVector()) without testing all the cases it affects is a bit risky. LGTM if that's confirmed safe or if the check is sunk to the cases you didn't mean to affect

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
143–144

Is removing this safe for the other cases of the switch (Opcode) below? Cases like G_ADD/G_SUB haven't been tested on vectors yet and this patch doesn't test them. It may be safer to sink this to each case and remove it as we test each one

Petar.Avramovic edited the summary of this revision. (Show Details)

Add a few unit tests and break for vector types on some opcodes.

arsenm added inline comments.Mar 3 2021, 6:13 AM
llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
233–234

This is impossible

240–241

This is impossible

455–456

This is impossible

arsenm requested changes to this revision.Mar 3 2021, 6:14 AM
This revision now requires changes to proceed.Mar 3 2021, 6:14 AM

Removed isVector checks for non-Vector opcodes. Use Register instead of unsigned.

arsenm accepted this revision.Mar 3 2021, 6:55 AM
This revision is now accepted and ready to land.Mar 3 2021, 6:55 AM
This revision was landed with ongoing or failed builds.Mar 4 2021, 6:06 AM
This revision was automatically updated to reflect the committed changes.

One of your 3 changes in https://github.com/llvm/llvm-project/compare/fe5c2c3ca682...bf5a58265047 broke check-llvm: http://45.33.8.238/linux/40834/step_12.txt

Please take a look for now, and revert if fixing takes a while.

Also, please spread out commits a bit more over time, so that when one breaks something, it's clearer which one it is :)

That failure was due to this change, so I reverted it in 4b1015361c16113e1b03c11af646d11522913705 . Please run tests before committing.

I've also reverted D96031 in 59beb1ef6d759eb63422507e8e8488502bcb62d8 since it depended on this change here.

...and D95432 in e68de60bc4f49e282756f1bc8072a196660e0fab since it also depended on this change if the AMDGPU target is enabled.