Address the commit https://reviews.llvm.org/D136015#inline-1313479
Details
Details
Diff Detail
Diff Detail
Event Timeline
| llvm/test/Transforms/InstCombine/mul.ll | ||
|---|---|---|
| 1587–1588 ↗ | (On Diff #469179) | 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. |
| 1695 ↗ | (On Diff #469179) | 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. |
| llvm/test/Transforms/InstCombine/mul.ll | ||
|---|---|---|
| 1587–1588 ↗ | (On Diff #469179) | 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
} |
| llvm/test/Transforms/InstCombine/mul.ll | ||
|---|---|---|
| 1587–1588 ↗ | (On Diff #469179) | I don't understand this comment. The example you show here should not be reduced: 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. |
Comment Actions
a) Add case with extra use to exercise those cases
b) Add some more negitive test cases and todo cases
| llvm/test/Transforms/InstCombine/mul.ll | ||
|---|---|---|
| 1587–1588 ↗ | (On Diff #469179) | Thanks for your suggestion, and apply your comment with extra uses |
| 1695 ↗ | (On Diff #469179) | Add extra uses for all 16 cases of variety types, thanks |
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 46 | Remove the extra use of the return value in all tests - that does not change anything for the transform. | |
| 73 | I think it would be better to not have extra uses of both hi-lo muls in all of the tests. Just keep the extra uses in half of the tests? We want to have some coverage for both cases: the mul uses the Lo value, or the mul uses the full in0/in1 directly. | |
Comment Actions
a) The 1st half tests have extra uses of both hi-lo muls to use the Lo value, and the 2nd half have no extra uses to uses the full in0/in1 directly.
b) delete all the extra use of the return value
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | so update with "the 2nd half have no extra uses of mul to uses the full in0/in1 directly" to avoid the conflict ? | |
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | 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: 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. | |
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | Sorry - I messed up the shift amounts when narrowing the width in those examples. Use this link instead: | |
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | 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** | |
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | I don't understand the question. I'd have:
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. | |
Comment Actions
4 tests that use in0/in1 with different commutes
4 tests that use in0/In1Lo with different commutes
4 tests that use In0Lo/in1 with different commutes
4 tests that use In0Lo/In1Lo with different commutes
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | As the initial version doesn't support the vector type and wide type, so I only add TODO cases for that (mul_v2i8_low and mul130_low), I'll add new cases with extra use in a follow-up patch when they are supported ? | |
Comment Actions
- update the CHECK-NEXT with update_test_checks.py
llvm/utils/update_test_checks.py --opt-binary=/e/source/llvm-project/build/bin/opt.exe llvm/test/Transforms/InstCombine/mul_fold.ll
Comment Actions
Add extra 2 cases (@mul130_low_one_extra_user and @mul_v2i8_low_one_extra_user) to coverage for the wide/vector types
| llvm/test/Transforms/InstCombine/mul_fold.ll | ||
|---|---|---|
| 304 | Thanks for patient, I added extra two cases for vector/wide types (@mul130_low_one_extra_user and @mul_v2i8_low_one_extra_user) | |
Comment Actions
This doesn't answer my request ("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.").
But go ahead and push this, and we can make adjustments as a follow-up quicker than continuing this review.
Remove the extra use of the return value in all tests - that does not change anything for the transform.