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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
"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. |
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). |
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. |
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.
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 |
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.
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