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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/test/CodeGen/X86/combine-bswap.ll:385
+}
+
 ; negative test
----------------
austin880625 wrote:
> RKSimon wrote:
> > goldstein.w.n wrote:
> > > goldstein.w.n wrote:
> > > > 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.
> > > ping on this comment, otherwise LG.
> > +1 for (1) - but I'd prefer equivalent tests added to combine-bitreverse.ll instead of being added to combine-bswap.ll
> 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
> 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.


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

https://reviews.llvm.org/D149782