[PATCH] D136340: [tests] precommit tests for D136015

Allen zhong via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 20 21:28:59 PDT 2022


Allen marked 2 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/mul.ll:1587-1588
+; CHECK-NEXT:    [[IN1HI:%.*]] = lshr i64 [[IN1]], 32
+; CHECK-NEXT:    [[M10:%.*]] = mul i64 [[IN1HI]], [[IN0]]
+; CHECK-NEXT:    [[M01:%.*]] = mul i64 [[IN0HI]], [[IN1]]
+; CHECK-NEXT:    [[M00:%.*]] = mul nuw i64 [[IN1LO]], [[IN0LO]]
----------------
spatel wrote:
> Allen wrote:
> > spatel wrote:
> > > In all of the tests with no extra uses, we are able to reduce the patterns to use the unmasked in0/in1 rather than In0Lo/In1Lo.
> > > 
> > > To get better test coverage, we need more variety of the patterns that actually use the Lo values, so add extra uses to more of these tests to exercise those cases.
> > > 
> > > You can vary the types (wider, narrower, vectors) in this set of 16 tests, so we don't need extra tests specifically for different types.
> > Now the following pattern can't be addressed with D136015, it need more work to match, maybe just as @RKSimon said in https://reviews.llvm.org/D136015#inline-1314325, so I hope this can be fixed out later with a separate patch?
> > ```
> > define i64 @mul64_low_A0_B0(i64 noundef %in0, i64 noundef %in1) {
> >   %In0Hi = lshr i64 %in0, 32
> >   %In1Hi = lshr i64 %in1, 32
> >   %m10 = mul i64 %In1Hi, %in0
> >   %m01 = mul i64 %in1, %In0Hi
> >   %m00 = mul i64 %in1, %in0
> >   %addc = add i64 %m10, %m01
> >   %shl = shl i64 %addc, 32
> >   %addc9 = add i64 %shl, %m00
> >   ret i64 %addc9
> > }
> > ```
> I don't understand this comment. The example you show here should not be reduced:
> https://alive2.llvm.org/ce/z/JLmNU5
> 
> If can remove the `and` from these patterns completely (because the high bits are known zero), then we should also be able to remove the `lshr` (because the result must be zero).
> 
> So include that as a negative test? We should also have negative tests where the shift amount is not exactly half width and the mask value is not exactly half width.
Thanks for your suggestion, and apply your comment with extra uses
Add new negative  case mul64_low_no_and, mul16_low_miss_shift_amount and mul8_low_miss_half_width


================
Comment at: llvm/test/Transforms/InstCombine/mul.ll:1695
+; CHECK-NEXT:    [[IN1HI:%.*]] = lshr i64 [[IN1]], 32
+; CHECK-NEXT:    [[M10:%.*]] = mul i64 [[IN1HI]], [[IN0]]
+; CHECK-NEXT:    [[M01:%.*]] = mul i64 [[IN0HI]], [[IN1]]
----------------
spatel wrote:
> We can see that the commuted instruction is not actually tested because it changed. You'll need to add something (extra uses?) to make sure the tests are providing the coverage for the expected pattern.
Add extra uses for all 16 cases of variety types, thanks


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

https://reviews.llvm.org/D136340



More information about the llvm-commits mailing list