[PATCH] D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D52100#1277915, @Sonicadvance1 wrote:

> In https://reviews.llvm.org/D52100#1277817, @dsanders wrote:
>
> > In https://reviews.llvm.org/D52100#1277769, @Sonicadvance1 wrote:
> >
> > > This commit breaks my 128bit instruction target.
> > >  I'm currently using a workaround locally for enabling APInt based encodings when your instruction encoding is >64bit.
> > >  The main issue with this approach is that getBinaryCodeForInstr completely falls apart.
> >
> >
> > Could you elaborate on this? getBinaryCodeForInstr() is for encoding instructions whereas FixedLenDecoderEmitter generates a decoder so I'm surprised that you're seeing a connection between getBinaryCodeForInstr() and this patch. Our out-of-tree target didn't use the FixedLenDecoderEmitter until after this patch made it possible but we've been using getBinaryCodeForInstr() for a very long time so I think there's unlikely to be a connection between the two. Additionally, not affecting encoding was a requirement for us as we're also working around the lack of >64-bit support in getBinaryCodeForInstr()
>
>
> I can only assume you aren't using a disassembler in your backend currently? That also breaks immediately.


Before this patch, there was no means to have a disassembler that used FixedLenDecoderEmitter to support >64-bit instructions. With this patch, we've been able to bring up a working and tested disassembler very quickly. We're still a long way from supporting 100% of our ISA but we're making progress very quickly and our experience is far from "breaks immediately". Our biggest problem is conflicting encodings in our tablegen definitions and we find instructions usually work once those decoder conflicts are resolved. The next biggest is that disassemblers with a lot of variety can be a bit slow to compile due to the number of allocas the APInt objects create (part of this is DCE being >=quadratic on number of allocas and exit blocks). To be clear, I fully expect that a mature target trying to make use of a new feature may hit issues that our target hasn't. That's unfortunately par for the course for targets making use of new features, especially when the target concerned is out of tree like both of ours.

Going back to the issue you're encountering: What do you mean by "breaks immediately"? I'd like to help you resolve the problem you're encountering but I need more than "it's broken" to go on. Does it crash? Pick the wrong opcode? Produce the wrong MCInst operands? or something else?

> Any form of reasonably mature backend is going to hit issues due to this being half baked at the moment.
>  I was using the getBinaryCodeForInstr function as an example of missing features

getBinaryCodeForInstr() is a conscious omission as I'm trying to get >64-bit instructions working for the MCDisassembler layer and getBinaryCodeForInstr() is not part of that layer (it's in the MC layer). We generally develop things incrementally in LLVM rather than try to deliver everything at once in one large patch. Among other reasons (which can be found at https://llvm.org/docs/DeveloperPolicy.html#incremental-development), this makes code review easier and also allows the community more opportunity to shape the overall direction and consider the viewpoints of the relevant targets to produce a better feature than would otherwise be possible.

In this case, the high-level increments are:

1. Make it possible to support >64-bit in the MCDisassembler layer
2. (out-of-tree) Use that support to implement a disassembler that requires it
3. Make it possible to support >64-bit in the MCCodeEmitter and related layers
4. (out-of-tree) Replace our various hacks with a proper implementation based on that support

Our target doesn't have an urgent need to do 3 and 4 yet (because we have out-of-tree workarounds) but we do want to resolve that too as soon as we can as it would improve the quality and reduce the maintenance requirements of our compiler.

> that this misses but there are definitely more than what I've stated.

If you are aware of any affecting the MCDisassembler layer (aside from the known issue of >64-bit fields) then I'd appreciate more information on this. It's entirely possible that there are things I haven't thought about or encountered yet

> Sadly my backend is out of tree so I can't complain too much.

I understand the difficulty here as our backend is also out-of-tree and we too occasionally have issues with upstream changes and find it difficult to share details when we need upstreams help to resolve it. We usually end up reproducing the issue using toy examples and an in-tree target to enable us to discuss it.


Repository:
  rL LLVM

https://reviews.llvm.org/D52100