[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:
> 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):


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:28
     else
       Offset = Op.getCImm()->getZExtValue();
 
----------------
(2) while we use getZExtValue here. Is this intended?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUGlobalISelUtils.cpp:36
     // TODO: Handle G_OR used for add case
     if (mi_match(Def->getOperand(2).getReg(), MRI, m_ICst(Offset)))
       return std::make_pair(Def->getOperand(1).getReg(), Offset);
----------------
(1) m_ICst uses getSExtValue() on APInt


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

https://reviews.llvm.org/D91336