[PATCH] D48025: [PowerPC] avoid masking already-zero bits in BitPermutationSelector

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked an inline comment as done.
inouehrs added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1319
       if (V.getValueType() != MVT::i64 ||
-          V.getOperand(0).getValueType() != MVT::i32)
+          V.getOperand(0).getValueSizeInBits() != 32)
         break;
----------------
hfinkel wrote:
> Why was this changed? I'm fine with it either way, but I think that we should be consistent. Either change the type for V and V.getOperand(0), or the number of bits for both.
I forgot to remove irrelevant test code unintentionally. Thank you for pointing this out.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1426
+      // when the first non-zero bit appears later.
+      if (IsGroupOfZeros && Bits[i].isZero())
+        continue;
----------------
hfinkel wrote:
> Couldn't you end up here with a single group that's a mixture of constant zeros and known-to-be-zero bits from different underlying values (with, potentially, different rotation factors)? If so, isn't that a problem?
Known-to-be-zero bits from multiple underlying values can be in the same group of zeros. It is ok if they are known to be zero. Masking bits and copying these (zero) bits from the underlying values should give the same result.
What we need to really care is not to include non-zero bits in a group of zeros, which is checked in line 1434 below.


https://reviews.llvm.org/D48025