[PATCH] D138661: [AMDGPU][MC] Correct handling of mandatory literals

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash accepted this revision.
Joe_Nash added a comment.
This revision is now accepted and ready to land.

Ok, apart from that comment LGTM.



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:3387
+  if (!AMDGPU::isSISrcOperand(Desc, OpIdx) ||
+      AMDGPU::isKImmOperand(Desc, OpIdx)) {
     return false;
----------------
dp wrote:
> Joe_Nash wrote:
> > dp wrote:
> > > dp wrote:
> > > > Joe_Nash wrote:
> > > > > This looks a bit weird to me because isSISrcOperand used to be true for KImm, until I created the mandatory literal logic. I'm not sure what the desired semantics of isSISrcOperand are. Can you try making isSISrcOperand used to be true for KImm and seeing if there is any fallout?
> > > > `isSISrcOperand` does return true for `KImm` and seems it always did. I just added a limitation on `KImm` operands - they can no longer be interpreted as inline constants.
> > > > 
> > > > Before the mandatory literal logic was introduced, this limitation on `KImm` operands was unnecessary because only one literal was allowed for affected instructions.
> > > > 
> > > > Could you describe your concern in more detail?
> > > Are you suggesting to see if it is feasible to make `isSISrcOperand` return false for `KImm`?
> > > Are you suggesting to see if it is feasible to make `isSISrcOperand` return false for `KImm`?
> > 
> > Yes.
> > 
> > Sorry, I said the reverse of what I meant. 
> I prepared a separate change on top of this patch to see how this can be done. Please take a look. I'm not sure if this would be that useful.
> 
> https://reviews.llvm.org/D139101
Thanks for trying that. I agree it doesn't look too useful.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h:1062
 
 /// Can this operand also contain immediate values?
 bool isSISrcOperand(const MCInstrDesc &Desc, unsigned OpNo);
----------------
Can you make this comment more precise?


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

https://reviews.llvm.org/D138661