[PATCH] D29933: [RISCV 11/n] Initial codegen support for ALU operations

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:146
+def ORI   : ALU_ri<0b110, "ori", or>;
+def ANDI  : ALU_ri<0b111, "andi", and>;
 
----------------
theraven wrote:
> This style is followed on the existing back ends to varying degrees, but I find that the mixing of instruction and pattern definitions makes it harder to follow the code when debugging.  The places where these are separate are usually a lot easier to spot mistakes (as a trivial example here, the instruction that will be selected for an OR DAG node is defined either on line 145 or on line 172, depending on the operand of the node.  Separating these definitions out [ideally into a separate RISCVPatterns.td file that's included] from the instruction definitions and placing them next to each other in the source makes it a lot easier to understand the logic and cleanly separates concerns).
This is an interesting idea. Instruction definitions are ordered to allow easy cross-referencing with the RISC-V specification, but as you point out this means that code generation patterns aren't always grouped in a natural way. The only downside from separating patterns is you can't see at a glance if a pattern might be missing (e.g. no pattern was given for ori) - but that would be caught by tests anyway.

I'll have a play and see what such a factoring would look like.


https://reviews.llvm.org/D29933