[PATCH] D52736: [AMDGPU] Fixed SIInstrInfo::getOpSize to handle subregs

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h:735-736
+    if (MO.isReg()) {
+      if (unsigned SubReg = MO.getSubReg())
+        return RI.getSubRegIndexLaneMask(SubReg).getNumLanes() * 4;
+    }
----------------
arsenm wrote:
> rampitec wrote:
> > arsenm wrote:
> > > rampitec wrote:
> > > > arsenm wrote:
> > > > > I think this silently will break if we start using 16-bit subregs. Can you assert that this number is consistent with the size of the super-reg?
> > > > In fact a lot will break if we start using 16 bit subregs. I would like to add the assert, but that is exactly the problem, one cannot know subreg's size without assuming target specific subreg layout. I.e. I do not see how to do it w/o listing all possible subregs in our target. Also note that MRI.getMaxLaneMaskForVReg() will happily return -1 for the whole register. Any ideas?
> > > You can just assert against the size of the subreg class? Something like (getRegSizeInBits(getSubClassWithSubReg(getRegClass(Reg)) >= 32)?
> > A subreg can be sub0_sub1, which will pass the check.
> Yes, that should work. The issue is if a subregindex like sub0_lo16 is used
As long as you are not planning to have sub0_lo16_hi16 ;) I see the point, at least cases will fail, which is sufficient to catch.


Repository:
  rL LLVM

https://reviews.llvm.org/D52736