This is an archive of the discontinued LLVM Phabricator instance.

ARMAsmParser: clean up of isImmediate functions
ClosedPublic

Authored by SjoerdMeijer on Mar 24 2017, 8:00 AM.

Details

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Mar 24 2017, 8:00 AM
olista01 edited edge metadata.Mar 24 2017, 9:43 AM

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.

Forgot to remove a commented out piece of code.

rengolin edited edge metadata.Apr 3 2017, 4:02 AM

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. :(

SjoerdMeijer added inline comments.Apr 3 2017, 5:15 AM
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.

rengolin added inline comments.Apr 3 2017, 5:58 AM
lib/Target/ARM/ARMInstrInfo.td
694

I'm happy for these cleanups to come in following patches.

olista01 accepted this revision.Apr 3 2017, 6:32 AM

LGTM

This revision is now accepted and ready to land.Apr 3 2017, 6:32 AM
This revision was automatically updated to reflect the committed changes.