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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked an inline comment as not done.
goldstein.w.n added inline comments.


================
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
----------------
goldstein.w.n wrote:
> 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`).
> 
> 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`).
> 




================
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
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > 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`).
> > 
> > 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`).
> > 
> 
> 
Sorry realized I accidentally submitted this incomplete.

I'm planning some future patches to handle more of the many missing
`bt{c|r|s}` cases. Handling this `cmpxchg` case (assuming it can be
done correctly and the `Trunc` isn't a dealbreaker) will involved
adding more cases to `FindSingleBitChange` and
`emitBitTestAtomicRMWIntrinsic`. My thinking submitting this patch
with out the baseline `1 << (c & SHIFT_MASK)` is to get some feedback
on the framework before increasing the complexity with a bunch of
additional cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140645