[PATCH] D135260: [AMDGPU][AMDGPULowerKernelAttributes] Use stripAndAccumulateConstantOffset

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some drive by comments



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:84
+  return M.getFunction(Name);
+}
+
----------------
This function and uses are unrelated.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:89
+    return L->isSimple() ? L : nullptr;
+  }
+
----------------
This does break with the "unique" property below. Two users, one simple one not, will result in a load. Is that expected?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:105
+  return UniqueLoad;
+}
+
----------------
A unique load reached via select or phi will not be detected. `UniqueLoad && UniqueLoad != L` can help with that.

Style: I'd remove braces for single return conditionals.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:138
       continue;
+    }
 
----------------
I would use the `Attributor::checkForAllUses`, also unsure why the loads need to be unique. Depending on  the call `AAPointerInfo` would allow you to handle way more things without any of this manual tracing. Either seems to be a useful rewrite beyond the scope of this. Feel free to ignore for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135260