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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/mul_fold.ll:303
+  %addc = add i32 %m10, %m01
+  call void @use32(i32 %addc)
+  %shl = shl i32 %addc, 16
----------------
Allen wrote:
> spatel wrote:
> > spatel wrote:
> > > Allen wrote:
> > > > spatel wrote:
> > > > > This extra use means we are still using In0Lo and In1Lo in the multiplies. The test comment above says this and the following tests should not be doing this?
> > > > > "the 2nd half have no extra uses to uses the full in0/in1 directly"
> > > > so update with "the 2nd half have no extra uses of mul to uses the full in0/in1 directly" to avoid the conflict ?
> > > No.
> > > 
> > > There are 4 variations of the partial multiplies: Hi/Lo and Hi/Lo, Hi/In and Hi/Lo, Hi/Lo and Hi/In, Hi/In and Hi/In.
> > > 
> > > The tests should be in canonical form to start - we do not want to rely on some other transform to produce the expected pattern for test.
> > > 
> > > Here are 2 examples of the tests and output that I expect to see:
> > > https://alive2.llvm.org/ce/z/C6ocXQ
> > > 
> > > We also want to test 16 commuted variations, so give each Hi/Lo variation 4 different commuted tests for maximum test coverage? If we want to be really thorough, we could just copy all of these patterns and get 16 * 4 = 64 tests, but I don't think that is necessary.
> > Sorry - I messed up the shift amounts when narrowing the width in those examples. Use this link instead:
> > https://alive2.llvm.org/ce/z/MkTpYs
> So I try to copy the 16 cases, delete the use of **call void @use32(i32 [[ADDC]])** and keep one use of mul similar as your link  **https://alive2.llvm.org/ce/z/MkTpYs** 
> i.e. one half only use **%m10** , and the other half only use **%m01** ? Does it enough? (we already have cases with no uses of mull and case with 2 uses of mull)
I don't understand the question. I'd have:
1. 4 tests that use in0/in1 with different commutes
2. 4 tests that use in0/In1Lo with different commutes
3. 4 tests that use In0Lo/in1 with different commutes
4. 4 tests that use In0Lo/In1Lo with different commutes

I'd vary the types across those 16 tests to include at least one vector type and one wide type, and that should provide coverage for all of the patterns that we are trying to fold.


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

https://reviews.llvm.org/D136340