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

[Jay Foad via Phabricator via llvm-commits]

foad 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) {
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:28
     else
       Offset = Op.getCImm()->getZExtValue();
 
----------------
Petar.Avramovic wrote:
> (2) while we use getZExtValue here. Is this intended?
getSExtValue vs getZExtValue affects how the 32-bit value is extended to int64_t. But if you make this function return a 32-bit result, then that doesn't matter.


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

https://reviews.llvm.org/D91336