llvm dev - Aug 2017 - RFC/bikeshedding: Separation of instruction and pattern definitions in LLVM backends

As many of you know, I have a growing series of patches for a RISC-V backend
under/awaiting review
<https://reviews.llvm.org/differential/?authors=asb&order=updated>,
<http://github.com/lowrisc/riscv-llvm>. I'll be posting a larger
status update
on that work either later today or tomorrow, this RFC focuses on an issue that
came up during review which I think may benefit from wider input.

David Chisnall suggested that the backend could be made easier to read and
work with by separating out instruction definitions from the patterns used to
match them for codegen. One key advantage of such a separation is that
patterns and insruction definitions can be grouped and ordered independently
in the way that makes most sense. Patterns for addi and add benefit from being
grouped together, but it may make more sense to group the reg-reg and reg-imm
instruction definitions separately. The main downside is that this style is
not quite as concise as specifying patterns alongside the instruction
definition. Repetition seems to be effectively reduced with a few simple Pat
classes.

Semantic information such as isBranch is still represented in the instruction
definition meaning there isn't a complete split between MC-layer and codegen
concerns. The Mips{64,32}r6InstrInfo.td does also factor out this information.
This seems less compelling to me, but dissenting opinions are welcome!

I've demonstrated both the "conventional" approach
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_conventional-td>
and the "separate patterns" approach
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_separate_pats-td>.
Obviously once patterns and pseudo-instructions are separated out, you may
want to move them to a different .td file.

Does anyone have strong views on these sort of choices one way or another?

Best,

Alex

On 18 Aug 2017, at 10:55, Alex Bradbury <asb at asbradbury.org>
wrote:
> 
> I've demonstrated both the "conventional" approach
>
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_conventional-td>
> and the "separate patterns" approach
>
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_separate_pats-td>.
> Obviously once patterns and pseudo-instructions are separated out, you may
> want to move them to a different .td file.
> 
> Does anyone have strong views on these sort of choices one way or another?
I do find the second easier to follow, though both are a lot easier to read than
the MIPS back end.

David

I agree with David's sentiment.  The second method appears to be easier to
follow.  IMHO, this would be easier for external users that desire to
modify the backend for their own custom extensions/instructions.

On Fri, Aug 18, 2017 at 5:05 AM, David Chisnall <David.Chisnall at
cl.cam.ac.uk
> wrote:
> On 18 Aug 2017, at 10:55, Alex Bradbury <asb at asbradbury.org>
wrote:
> >
> > I've demonstrated both the "conventional" approach
> > <https://gist.github.com/asb/0c61ebc131076c6186052c29968a49
> 1d#file-riscvinstrinfo_conventional-td>
> > and the "separate patterns" approach
> > <https://gist.github.com/asb/0c61ebc131076c6186052c29968a49
> 1d#file-riscvinstrinfo_separate_pats-td>.
> > Obviously once patterns and pseudo-instructions are separated out, you
> may
> > want to move them to a different .td file.
> >
> > Does anyone have strong views on these sort of choices one way or
> another?
>
> I do find the second easier to follow, though both are a lot easier to
> read than the MIPS back end.
>
> David
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170818/4d43e503/attachment-0001.html>

On 8/18/2017 4:55 AM, Alex Bradbury wrote:
> 
> David Chisnall suggested that the backend could be made easier to read and
> work with by separating out instruction definitions from the patterns used
to
> match them for codegen.
We have them separated on Hexagon for different reasons and it looks a 
lot cleaner that way.

-Krzysztof

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

One thing to be aware of with this is that (IIRC) tablegen uses the pattern to
infer things about the pattern. One example I vaguely remember is that an empty
pattern would result in the same effect as hasSideEffects=1 and I think there
were others.
> Semantic information such as isBranch is still represented in the
instruction
> definition meaning there isn't a complete split between MC-layer and
codegen
> The Mips{64,32}r6InstrInfo.td does also factor out this information.
> This seems less compelling to me, but dissenting opinions are welcome!
Mips doesn't manage a complete split between the MC-layer and CodeGen either
because there's overlap in the fields that they use. It's separating the
encoding from the operation and the availability. The encoding and availability
portions are focused on making them easily verifiable against the specification,
while the operation section deals with the description of the behaviour of the
instruction, how it maps to assembly, etc.
> On 18 Aug 2017, at 10:55, Alex Bradbury via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> As many of you know, I have a growing series of patches for a RISC-V
backend
> under/awaiting review
>
<https://reviews.llvm.org/differential/?authors=asb&order=updated>,
> <http://github.com/lowrisc/riscv-llvm>. I'll be posting a larger
status update
> on that work either later today or tomorrow, this RFC focuses on an issue
that
> came up during review which I think may benefit from wider input.
> 
> David Chisnall suggested that the backend could be made easier to read and
> work with by separating out instruction definitions from the patterns used
to
> match them for codegen. One key advantage of such a separation is that
> patterns and insruction definitions can be grouped and ordered
independently
> in the way that makes most sense. Patterns for addi and add benefit from
being
> grouped together, but it may make more sense to group the reg-reg and
reg-imm
> instruction definitions separately. The main downside is that this style is
> not quite as concise as specifying patterns alongside the instruction
> definition. Repetition seems to be effectively reduced with a few simple
Pat
> classes.
> 
> Semantic information such as isBranch is still represented in the
instruction
> definition meaning there isn't a complete split between MC-layer and
codegen
> concerns. The Mips{64,32}r6InstrInfo.td does also factor out this
information.
> This seems less compelling to me, but dissenting opinions are welcome!
> 
> I've demonstrated both the "conventional" approach
>
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_conventional-td>
> and the "separate patterns" approach
>
<https://gist.github.com/asb/0c61ebc131076c6186052c29968a491d#file-riscvinstrinfo_separate_pats-td>.
> Obviously once patterns and pseudo-instructions are separated out, you may
> want to move them to a different .td file.
> 
> Does anyone have strong views on these sort of choices one way or another?
> 
> Best,
> 
> Alex
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 21 August 2017 at 11:53, Daniel Sanders <daniel_l_sanders at apple.com>
wrote:
> One thing to be aware of with this is that (IIRC) tablegen uses the pattern
to infer things about the pattern. One example I vaguely remember is that an
empty pattern would result in the same effect as hasSideEffects=1 and I think
there were others.
Thanks for the note - excellent point. Looking at
CodeGenDAGPatterns.cpp, it seems in the absence of a pattern
hasSideEffects will be 1, while mayLoad and mayStore default to 0.
Back in 2012, Jakob Stoklund Olesen added the
guessInstructionProperties flag, which causes an error
<https://reviews.llvm.org/rL162460> if a property isn't set explicitly
and can't be inferred. It doesn't look like any other in-tree targets
have ended up enabling this, but it looks like it would be worth
enabling for RISCV, particularly if going ahead with splitting
instructions and patterns.

Best,

Alex