[PATCH] D150347: [AMDGPU] Improve abs modifier usage

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks good overall.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp:5148
+    // Ensure we are not already sinking this operand.
+    if (any_of(Ops, [&](Use *U) { return U->get() == Op; }))
+      continue;
----------------
foad wrote:
> tsymalla wrote:
> > foad wrote:
> > > What does this do? Did you mean to check basic blocks here? In any case I am not sure this is required.
> > It checks whether one of the operands is registered twice, which appears like common practice amongst other implementations as well 
> Oh I see. Thanks.
Nit: I find this confusing because `U` and `Op` are both `Use`s but you only call `get` on one of them. I guess it relies on an implicit conversion for the other one. I think `U->get() == Op.get()` would be clearer.


================
Comment at: llvm/test/CodeGen/AMDGPU/andorbitset.ll:56
 ; SI-NOT:   %bb.1:
-; SI:       s_bitset0_b32 s{{[0-9]+}}, 31
+; SI:       s_and_b32 s{{[0-9]+}}, s{{[0-9]+}}, 0x7fffffff
 define void @bitset_verifier_error() local_unnamed_addr #0 {
----------------
I guess we no longer form s_bitset because the s_and uses two different registers? That means that this is no longer testing the original bug from D85307. Maybe change the test to use `and i32 undef, 0xBFFFFFFF` (clear bit 30) instead of fabs, so that your new optimization won't spoil it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150347