[PATCH] D140645: Add tests for atomic bittest with register/memory operands

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D140645#4015865 <https://reviews.llvm.org/D140645#4015865>, @pengfei wrote:

> The test is so large! Are we over testing for this feature?
> Beside, split the test to another patch and only lieve diff here.





================
Comment at: llvm/test/CodeGen/X86/atomic-rm-bit-test.ll:2-6
+; RUN: llc -mtriple=i686-unknown-linux-gnu -mattr=-bmi,-bmi2 < %s | FileCheck %s --check-prefixes=X86-NOBMI2
+; RUN: llc -mtriple=i686-unknown-linux-gnu -mattr=+bmi,-bmi2 < %s | FileCheck %s --check-prefixes=X86-NOBMI2
+; RUN: llc -mtriple=i686-unknown-linux-gnu -mattr=+bmi,-bmi2 < %s | FileCheck %s --check-prefixes=X86-NOBMI2
+; RUN: llc -mtriple=i686-unknown-linux-gnu -mattr=+bmi,+bmi2 < %s | FileCheck %s --check-prefixes=X86-BMI2
+; RUN: llc -mtriple=i686-unknown-linux-gnu -mattr=+bmi,+bmi2 < %s | FileCheck %s --check-prefixes=X86-BMI2
----------------
pengfei wrote:
> Does having `bmi/bmi2` affect the selection to bts?
It may affect the `_*val` tests which return the original `AtomicRMW` value as is. I.e `return __atomic_fetch_xor(p, 1 << c, __ATOMIC_RELAXED) & (1 << c)` b.c that gets a `shlx` vs `shl` in codegen. Can drop if you think its unnecessary.


================
Comment at: llvm/test/CodeGen/X86/atomic-rm-bit-test.ll:13
+
+define i16 @atomic_shl1_xor_16_gpr_val(ptr %v, i16 %c) {
+; X86-NOBMI2-LABEL: atomic_shl1_xor_16_gpr_val:
----------------
pengfei wrote:
> Use `nounwind` to suppress the cfi* directives.
Will do for V2 (going to wait for reply on some other comments first as it seems the tests might go to another PR / some of them may be dropped).


================
Comment at: llvm/test/CodeGen/X86/atomic-rm-bit-test.ll:30
+; X86-NOBMI2-NEXT:    # kill: def $ax killed $ax killed $eax
+; X86-NOBMI2-NEXT:    lock cmpxchgw %cx, (%esi)
+; X86-NOBMI2-NEXT:    # kill: def $ax killed $ax def $eax
----------------
pengfei wrote:
> Why still generating `cmpxchg`?
So there are a lot of missing cases in `AtomicRMW` ->
`bt{c|r|s}`. (The code has comments for many of the cases).

The IR for this is `Trunc(Shl(1, shift_cnt))`. This is the natural
codegen for C-code doing `1 << c` b.c its UB for `c >= 16` but at the
point where we are checking if we can use atomic bittest I wasn't sure
we could actually be sure that it couldn't be an intentional
side-affect for `Trunc(Shl(1, shift_cnt))` to be zero for `shift_cnt>= 16` (if say this IR was from some language where shifting by >=
type width was defined as returning zero).

This can be changed fairly easily by modify `FindSingleBitChange` if
the reasoning above is incorrect. Although maybe in another patch?
There are alot of missing cases for all types (alot get messed up by
`InstrCombinePass`).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140645