[PATCH] D91336: AMDGPU/GlobalISel: Fix negative offset folding for buffer_load

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:16
 
-std::pair<Register, unsigned>
+std::pair<Register, int64_t>
 AMDGPU::getBaseWithConstantOffset(MachineRegisterInfo &MRI, Register Reg) {
----------------
foad wrote:
> Petar.Avramovic wrote:
> > foad wrote:
> > > I am really not sure about this change. I think `Reg` is always a 32-bit value here (perhaps we should assert this), so any G_ADD that we decompose will be a 32-bit add, so using `unsigned` (or perhaps `int`) for the offset makes sense.
> > There is range checking for the offset so the container for value should not make big difference. Choosing int or unsigned brings ambiguity for '32-bit values with 1 at highest bit' (I would assume that it is most probably negative offset then large positive offset so int could work). With int64 we know if value was positive or negative in ir. 
> > 
> > Also see below (1), (2):
> > With int64 we know if value was positive or negative in ir.
> 
> No, it's just a 32-bit value, in IR and in MIR. You don't get any extra information by returning int64_t.
I meant that APInt and int64_t (used by G_CONSTANT) keep sign info and we know what was intended use of that 32 bit value.
Given 32 bit value 0xFFFFFFFC, we can't handle it here if it was used as negative offset (-4 as int64_t) since soffset behaves as unsigned in address calculation. However, if that is a large positive(unsigned) offset (4294967292 as int64_t) folding this into soffset behaves correctly. 


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

https://reviews.llvm.org/D91336