[PATCH] D31333: ARMAsmParser: clean up of isImmediate functions

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/ARM/ARMInstrInfo.td:694
+def Imm8_255AsmOperand: ImmAsmOperand<8,255> { let Name = "Imm8_255"; }
+def imm8_255 : Operand<i32>, ImmLeaf<i32, [{
+  return Imm >= 8 && Imm < 256;
----------------
rengolin wrote:
> Why is this one different? Why does it need the check inline, unlike the others?
Thanks for reviewing. There are quite some subtleties here. For example, some instructions encode  immediate values as "value - 1", and then it needs to print them out as "value + 1". I believe this was one these cases, and using the AsmOperand class here messes up that logic a bit. I fully agree that it would be nicer and more consistent to have a similar approach here. But as you also wrote in the other remark, I would like to address these things in follow up commits as this commit is already quite large; thought this would be a good point to stop as a first clean up.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9092
+  case Match_ImmRange0_3:
+    return Error(ErrorLoc, "immediate operand must be in the range [0,3]");
+  case Match_ImmRange0_7:
----------------
rengolin wrote:
> It's a shame you can't put all those `Match_*` into one match error with different parameters. :(
I believe one of Oliver's patches will tablegen this so we don't have to do this by hand, which is indeed not very nice. 


https://reviews.llvm.org/D31333