[PATCH] D83394: [AMDGPU] Avoid splitting FLAT offsets in unsafe ways

[Jay Foad via Phabricator via llvm-commits]

foad marked an inline comment as done.
foad added a comment.

> The mirror change is needed for globalisel

AMDGPUInstructionSelector::selectFlatOffsetImpl doesn't attempt to split offsets so I don't think there's anything to fix.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1705
+          // Use signed division by a power of two to truncate towards 0.
+          int64_t D = 1LL << (NumBits - 1);
+          RemainderOffset = (static_cast<int64_t>(COffsetVal) / D) * D;
----------------
arsenm wrote:
> arsenm wrote:
> > foad wrote:
> > > arsenm wrote:
> > > > This limitation also only needs to be applied if AS == FLAT_ADDRESS
> > > The only "limitation" is that we don't try to split negative offsets if the immediate offset field is unsigned, but you're saying we can do that if AS != FLAT_ADDRESS? What would that mean - that we're using a FLAT instruction but we know statically which part of the address space it is accessing??
> > Correct. This is always the case pre-gfx9 which did not have the "global" flat instructions
> Actually pre-gfx9 also didn't have flat offsets. However gfx10 does have a bug with flat offsets, so I think it would still be correct to model this correctly. The instruction patterns do accept either (and global instructions are only preferred through pattern priority)
> This limitation also only needs to be applied if AS == FLAT_ADDRESS

I still don't get this. Surely if we're using a FLAT instruction, even if we know which specific address space the programmer is trying to access, we still have to avoid setting vaddr to an address that might point into the wrong aperture.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83394