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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Whatever we end up doing for fabs, we should do the same for fneg and fneg-fabs. (But that doesn't have to be part of this patch.)



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp:223-225
+  // Early opt-out as we don't want to operate on undef or poison values.
+  if (Intrinsic && hasUndefOrPoisonOperand(*Intrinsic))
+    return false;
----------------
Why do you need this special case at all? And why only for intrinsics?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp:232-234
+    Instruction *UI = cast<Instruction>(U);
+    if (!UI)
+      continue;
----------------
All users of Instructions are Instructions, so this cast will always succeed. (And anyway if you want a cast that can fail, you need to use `dyn_cast`.)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp:247
+    // phi nodes, so ignore these cases as well.
+    if (IsPhi || !DT || !DT->dominates(I, UI)) {
+      CanErase = false;
----------------
Surely the "dominates" test can never fail, since IR uses SSA form?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp:253
+    if (Intrinsic) {
+      // Don't generate fabs(fabs(x)) calls.
+      if (auto *UII = dyn_cast<IntrinsicInst>(UI);
----------------
I don't think you need a special case for this. Any fabs(fabs(x)) - even in different basic blocks - should already have been simplified by generic IR optimizations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150347