This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Add bswap(logic_op(bswap(x), y)) regression test case; NFC
ClosedPublic

Authored by austin880625 on May 3 2023, 1:42 PM.

Details

Summary

Fold the following case on SelectionDAG combiner
This patch includes the regression test cases

bswap(logic_op(x, bswap(y))) -> logic_op(bswap(x), y)
bswap(logic_op(bswap(x), y)) -> logic_op(x, bswap(y))
bswap(logic_op(bswap(x), bswap(y))) -> logic_op(x, y) (with multiuse)

Diff Detail

Event Timeline

austin880625 created this revision.May 3 2023, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 1:42 PM
austin880625 requested review of this revision.May 3 2023, 1:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2023, 1:42 PM

include full context

goldstein.w.n added inline comments.May 3 2023, 2:28 PM
llvm/test/CodeGen/X86/combine-bswap.ll
328

youre *_rhs_* still have the bswap on the lhs.

386

Can you do two things.

  1. Add a negative test that is (bswap (logic (bitreverse a), b))
  2. Make some of the tests use bitreverse. You don't need to add new tests, just modify half the bswap ones to use `bitreverse instead.

Maybe xor + or bitlogic coverage? You don't need to repeat the tests you've written, just convert some to use the other ops instead

austin880625 edited the summary of this revision. (Show Details)

Update xor, or logical ops coverage in the recently added test cases

goldstein.w.n added inline comments.May 4 2023, 8:31 PM
llvm/test/CodeGen/X86/combine-bswap.ll
386

ping on this comment, otherwise LG.

RKSimon added inline comments.May 5 2023, 7:05 AM
llvm/test/CodeGen/X86/combine-bswap.ll
386

+1 for (1) - but I'd prefer equivalent tests added to combine-bitreverse.ll instead of being added to combine-bswap.ll

add bitreverse test cases and a bswap, bitreverse mixed negative test case

austin880625 added inline comments.May 5 2023, 1:45 PM
llvm/test/CodeGen/X86/combine-bswap.ll
386

I was trying to add (bitreverse (logic (bswap a), b)) in combine-bitreverse.ll and (bswap (logic (bitreverse a), b)) in combine-bswap.ll and found the following:

x86 doesn't have RBIT like ARM, so the first instruction of bitreverse is a bswap(to replace 0x00FF00FF), then the 0x0F0F0F0F, 0x33333333,... twiddling. This cause the (bitreverse (logic (bswap a), b)) actually got optimized as a negative test, just not on the major part of bitreverse.

; X86-LABEL: brev_xor_rhs_bs32:                                                      
; X86:       # %bb.0:                                                                
; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
; X86-NEXT:    bswapl %eax ; <--- this becomes LHS
; X86-NEXT:    xorl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    bswapl %eax <---- this line got removed
; X86-NEXT:    movl %eax, %ecx
; X86-NEXT:    andl $252645135, %ecx # imm = 0xF0F0F0F
; X86-NEXT:    shll $4, %ecx
...

Currently I only keep (bswap (logic (bitreverse a), b)) for this patch, then there's no diff after the optimization as desired, though this assumes the order of bswap and the manual bit ops.

Another way is moving these test cases into ARM version(need to create new files), which also reduces a lot of FileCheck lines

goldstein.w.n added inline comments.May 5 2023, 4:16 PM
llvm/test/CodeGen/X86/combine-bswap.ll
386

I was trying to add (bitreverse (logic (bswap a), b)) in combine-bitreverse.ll and (bswap (logic (bitreverse a), b)) in combine-bswap.ll and found the following:

x86 doesn't have RBIT like ARM, so the first instruction of bitreverse is a bswap(to replace 0x00FF00FF), then the 0x0F0F0F0F, 0x33333333,... twiddling. This cause the (bitreverse (logic (bswap a), b)) actually got optimized as a negative test, just not on the major part of bitreverse.

@RKSimon maybe we should postpone x86 bitreverse lowing till after legalization?

; X86-LABEL: brev_xor_rhs_bs32:                                                      
; X86:       # %bb.0:                                                                
; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
; X86-NEXT:    bswapl %eax ; <--- this becomes LHS
; X86-NEXT:    xorl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    bswapl %eax <---- this line got removed
; X86-NEXT:    movl %eax, %ecx
; X86-NEXT:    andl $252645135, %ecx # imm = 0xF0F0F0F
; X86-NEXT:    shll $4, %ecx
...

Currently I only keep (bswap (logic (bitreverse a), b)) for this patch, then there's no diff after the optimization as desired, though this assumes the order of bswap and the manual bit ops.

Another way is moving these test cases into ARM version(need to create new files), which also reduces a lot of FileCheck lines

Putting the test file in the arm backend would be fine.

move bitreverse related test cases into ARM backend for rbit instruction support

add positive test cases in ARM backend

Applying: Add bswap(logic_op(bswap(x), y)) regression test case; NFC
.git/rebase-apply/patch:127: new blank line at EOF.
+
.git/rebase-apply/patch:220: new blank line at EOF.
+
warning: 2 lines add whitespace errors.

Can you fix the whitespace issues?

This revision is now accepted and ready to land.May 15 2023, 6:58 PM