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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/include/llvm/ADT/APInt.h:2251
 ///
 /// TODO: Do we need a mode where all bits must be set when merging down?
+APInt ScaleBitMask(const APInt &A, unsigned NewBitWidth, bool Down = false);
----------------
craig.topper wrote:
> This comment uses the word "down" as well, but to mean the number of bits is decreasing.  I believe this patch is implementing this TODO.
So, let me remove this TODO, thanks!


================
Comment at: llvm/lib/Support/APInt.cpp:2996
   } else {
     // Merge bits - if any old bit is set, then set scale equivalent new bit.
     unsigned Scale = OldBitWidth / NewBitWidth;
----------------
pengfei wrote:
> Is the comment correct after your change?
Good catch! Let me update the old comment.


================
Comment at: llvm/test/CodeGen/X86/splat-value.ll:20-23
+  %fshl = call <8 x i64> @llvm.fshl.v8i64(<8 x i64> %unmaskedload270, <8 x i64> %unmaskedload, <8 x i64> <i64 12, i64 12, i64 12, i64 12, i64 12, i64 12, i64 12, i64 12>)
+  %res = shufflevector <8 x i64> %fshl, <8 x i64> zeroinitializer, <8 x i32> <i32 0, i32 1, i32 10, i32 11, i32 12, i32 13, i32 6, i32 7>
+  store <8 x i64> %res, <8 x i64>* %arr, align 64
+  ret void
----------------
pengfei wrote:
> Where's `undef` in the test?
It will generated in the mid of DAG combining.


================
Comment at: llvm/test/CodeGen/X86/splat-value.ll:31
+attributes #0 = { nounwind readnone "target-cpu"="cannonlake" }
+attributes #1 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
----------------
LuoYuanke wrote:
> Do we need thoes attributes?
My copy it from a runfail test, let me recheck it a little later. (Sever can not log on now)


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

https://reviews.llvm.org/D128570