This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Fix always true condition in combineShiftToMULH
ClosedPublic

Authored by jmmartinez on Dec 7 2022, 9:07 AM.

Details

Summary

I double checked and it seems the condition is already covered by the tests that are already present

Diff Detail

Event Timeline

jmmartinez created this revision.Dec 7 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:07 AM
jmmartinez requested review of this revision.Dec 7 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 9:07 AM
jmmartinez edited the summary of this revision. (Show Details)Dec 7 2022, 9:09 AM
jmmartinez added a reviewer: RKSimon.

Looking at D133768 it looks like you could add additional test case to dagcomb-mullohi.ll which has 2 'upper bit only' users (srl and sra)?

jmmartinez added a comment.EditedDec 12 2022, 5:12 AM

Looking at D133768 it looks like you could add additional test case to dagcomb-mullohi.ll which has 2 'upper bit only' users (srl and sra)?

I'm not sure I follow. Isn't that case covered by the following function in the test case?

define i32 @mul_one_bit_hi_hi_u32(i32 %arg, i32 %arg1, i32* %arg2) {
  bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = lshr i64 %i4, 33
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

Or you mean having a srl and a sra in the same test-case?

define i32 @foo(i32 %arg, i32 %arg1, i32* %arg2) {
  bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = ashr i64 %i4, 33                  <-- change from logic to arithmetic
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

PS: Sorry for the delay, I was out-of-office.

Or you mean having a srl and a sra in the same test-case?

Yes this is the test case I think we need to ensure we have a test case that has different behaviour for the if statement - if you precommit that srl+sra test then and rebase this patch the test codegen should change to match.

Or you mean having a srl and a sra in the same test-case?

Yes this is the test case I think we need to ensure we have a test case that has different behaviour for the if statement - if you precommit that srl+sra test then and rebase this patch the test codegen should change to match.

I thought the same, but I cannot seem to trigger the bug (my guess is that it's getting saved by some other pattern).

For this test case, the generated assembly is the same before and after the fix.

define i32 @mul_one_bit_hi_hi_u32_asr(i32 %arg, i32 %arg1, i32* %arg2) {
; CHECK-LABEL: mul_one_bit_hi_hi_u32_asr:
; CHECK:       ; %bb.0: ; %bb
; CHECK-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
; CHECK-NEXT:    v_mul_hi_u32 v4, v1, v0
; CHECK-NEXT:    v_ashrrev_i64 v[0:1], 33, v[3:4]
; CHECK-NEXT:    flat_store_dword v[2:3], v4
; CHECK-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
; CHECK-NEXT:    s_setpc_b64 s[30:31]
bb:
  %i = zext i32 %arg to i64
  %i3 = zext i32 %arg1 to i64
  %i4 = mul nsw i64 %i3, %i
  %i5 = lshr i64 %i4, 32
  %i6 = trunc i64 %i5 to i32
  store i32 %i6, i32* %arg2, align 4
  %i7 = ashr i64 %i4, 33
  %i8 = trunc i64 %i7 to i32
  ret i32 %i8
}

OK - please can you add the test case to this patch so we at least have a canary if the other combine ever changes?

Added test case

RKSimon accepted this revision.Dec 14 2022, 5:28 AM

LGTM with a couple of minors - cheers!

llvm/test/CodeGen/AMDGPU/dagcomb-mullohi.ll
149 ↗(On Diff #482760)

mul_one_bit_hi_hi_u32_lshr_ashr ?

169 ↗(On Diff #482760)

add newline

This revision is now accepted and ready to land.Dec 14 2022, 5:28 AM
This revision was landed with ongoing or failed builds.Dec 15 2022, 4:05 AM
This revision was automatically updated to reflect the committed changes.