First attempt to clean up a bit the isImmediate functions.
Details
Diff Detail
Event Timeline
Could you set the PredicateMethods in the tablegen files like you did for AArch64, so that we don't need all of these stub functions at all?
Thanks for the suggestion!
I have rewritten the patch:
- It is now using immediate AsmOperands so that the range check functions are tablegen'ed.
- Big bonus is that error messages become much more accurate, i.e. instead of a useless "invalid operand" error message it will not say that the immediate operand must in range [x,y], which is why regression tests needed updating.
More tablegen operand descriptions could probably benefit from using immediateAsmOperand, but this is a first good step to get rid of most of the nearly identical range check functions. I will address the remaining immediate operands in next clean ups.
Hi Sjord,
Nice cleanup! Thanks!
I have a few general inline remarks, but overall, the patch looks good. I'll let Oliver finish his review, and I'm happy when he is.
cheers,
--renato
lib/Target/ARM/ARMInstrInfo.td | ||
---|---|---|
694 | Why is this one different? Why does it need the check inline, unlike the others? | |
lib/Target/ARM/AsmParser/ARMAsmParser.cpp | ||
950 | Is it possible to clean up the negs in the same way? Maybe for a different patch, though, as this one is quite big already. | |
9092 | It's a shame you can't put all those Match_* into one match error with different parameters. :( |
lib/Target/ARM/ARMInstrInfo.td | ||
---|---|---|
694 | 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. | |
lib/Target/ARM/AsmParser/ARMAsmParser.cpp | ||
9092 | 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. |
lib/Target/ARM/ARMInstrInfo.td | ||
---|---|---|
694 | I'm happy for these cleanups to come in following patches. |
Why is this one different? Why does it need the check inline, unlike the others?