[PATCH] D55568: AMDGPU: Don't peel of the offset if the resulting base could possibly be negative in Indirect addressing.

[Changpeng Fang via Phabricator via llvm-commits]

cfang marked 4 inline comments as done.
cfang added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1459
+    // the base (n0) to be negative.
+    if (CurDAG->SignBitIsZero(N0) || C1->getSExtValue() <= 0) {
+      Base = N0;
----------------
arsenm wrote:
> arsenm wrote:
> > Why does the constant itself matter here? Needs a test with a negative offset
> You should also make the cheaper check of the constant first. Why isn't this offset > 0 && SignBitIsZero?
If the offset <=0,  base >= (base + offset). It won't make base negative to peel of the offset if (base + offset) is non-negative.
There are already a few negative offset test cases in indirect-addressing-si.ll, and the output won't change with this patch.


================
Comment at: lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1459
+    // the base (n0) to be negative.
+    if (CurDAG->SignBitIsZero(N0) || C1->getSExtValue() <= 0) {
+      Base = N0;
----------------
cfang wrote:
> arsenm wrote:
> > arsenm wrote:
> > > Why does the constant itself matter here? Needs a test with a negative offset
> > You should also make the cheaper check of the constant first. Why isn't this offset > 0 && SignBitIsZero?
> If the offset <=0,  base >= (base + offset). It won't make base negative to peel of the offset if (base + offset) is non-negative.
> There are already a few negative offset test cases in indirect-addressing-si.ll, and the output won't change with this patch.
will change the order of checking as offset <=0 || SignBitIsZero


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

https://reviews.llvm.org/D55568