[PATCH] D64830: [Xtensa 4/10] Add basic *td files with Xtensa architecture description.

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr added inline comments.


================
Comment at: llvm/lib/Target/Xtensa/Xtensa.td:1
+//===- Xtensa.td - Describe the Xtensa Target Machine -----------*- tablegen -*-===//
+//
----------------
craig.topper wrote:
> Please limit line length to 80 characters
Corrected


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrFormats.td:1
+//===- XtensaInstrFormats.td - Xtensa Instruction Formats -------*- tablegen -*-===//
+//
----------------
craig.topper wrote:
> Please limit line length to 80 characters
Corrected


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrFormats.td:14
+                 InstrItinClass itin = NoItinerary>: Instruction
+{
+  let Namespace = "Xtensa";
----------------
craig.topper wrote:
> I think the prevailing style in LLVM has the open curly brace at the end of the previous line.
Corrected for instruction descriptions, format descriptions and instruction operands.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrFormats.td:31
+                   InstrItinClass itin = NoItinerary>:
+                   XtensaInst<3, outs, ins, asmstr, pattern, itin>
+{
----------------
craig.topper wrote:
> It might just be me, but I find this lining the XtensaInst up with template parameters hard to read.  I think a common style is to put at the start of the line with only a small indentation. For example
> 
> ```
> class PseudoI<dag oops, dag iops, list<dag> pattern>                                                                                                                                                                                                                                   
>   : X86Inst<0, Pseudo, NoImm, oops, iops, ""> {                                                                                                                                                                                                                                        
>   let Pattern = pattern;                                                                                                                                                                                                                                                               
> }       
> ```
@craig.topper , I corrected instruction formats and instruction descriptions, do you think that this code now in common style or it must be corrected bit more?


================
Comment at: llvm/lib/Target/Xtensa/XtensaOperands.td:1
+//===- XtensaOperands.td - Xtensa instruction operands ----*- tblgen-*--===//
+//
----------------
craig.topper wrote:
> This line seems shorter than 80 characters.
Corrected


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64830/new/

https://reviews.llvm.org/D64830