[PATCH] D92999: [amdgpu] Enhance load widening in the constant address space.

[Michael Liao via Phabricator via llvm-commits]

hliao added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7981-7983
+    // Skip pseudo source value.
+    if (!P)
+      return SDValue();
----------------
arsenm wrote:
> PseudoSourceValues can be permissible. However the first situations I think of at the top of my head are all cases where we would have emitted the correct code in the first place
Yeah, as pseudosource value is generated by the backend, where optimal code should be generated from the beginning. If there are cases introduced later, we may consider to enhance that original code and extend the support here.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:7985-7986
+    int64_t Offset = 0;
+    const Value *Base =
+        GetPointerBaseWithConstantOffset(P, Offset, DAG.getDataLayout());
+    KnownBits Known = computeKnownBits(Base, DAG.getDataLayout());
----------------
arsenm wrote:
> I'm not 100% comfortable relying on the IR value here. Can you just use the DAG known bits?
Here's the tradeoff I have to make based on the current alias interface in the backend. The alias checking in MI has the assumption that the `MachineMemOperand` offset should never be any negative offsets. But, the transformation here needs to rebase the pointer by a negative one to be DWORD-aligned. Without breaking the assumption so far, we need to adjust IR mapping to ensure the correctness of alias checking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92999