[PATCH] D128570: [ISel] Round down mask bit when merge undef(s) for DAG combine

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

OK, let me add test coverage in APIntTest.cpp, many thanks!



================
Comment at: llvm/include/llvm/ADT/APInt.h:2252
 /// TODO: Do we need a mode where all bits must be set when merging down?
-APInt ScaleBitMask(const APInt &A, unsigned NewBitWidth);
+APInt ScaleBitMask(const APInt &A, unsigned NewBitWidth, bool Down = false);
 } // namespace APIntOps
----------------
RKSimon wrote:
> Yes, 'down' in the TODO meant when reducing the number of bits - maybe 'MatchAllBits' would be better? Current behaviour is 'MatchAnyBits'.
Yes, MatchAllBits is better, thanks!


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2723
+        // We can not convert t2 to {i64 undef, i64 undef}
+        UndefElts |= APIntOps::ScaleBitMask(SubUndefElts, NumElts, true);
       }
----------------
RKSimon wrote:
> I regret using the term 'UndefElts' - what I meant was that these elements shouldn't be trusted.
: ) This is useful for undef optmization.
So In this case (the lit test I write) the UndefElts is mainly used for undef propagation. Now we give out a choose to select MatchAllBits or MatchAnyBits for it.
For BitCast, I think we should Conservatively use MatchAllBits.


================
Comment at: llvm/test/CodeGen/X86/splat-value.ll:4
+
+define void @do_pmadd52_hi(<8 x i64> %unmaskedload, <8 x i64> %unmaskedload270, <8 x i64>* %arr) #0 {
+; CHECK-LABEL: do_pmadd52_hi:
----------------
RKSimon wrote:
> Maybe a better test name - and a comment describing what you're testing for?
OK, this test is about fshl, let me rename it to "define void @test_fshl"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128570/new/

https://reviews.llvm.org/D128570