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

[Sterling Augustine via Phabricator via llvm-commits]

saugustine requested changes to this revision.
saugustine added a comment.
This revision now requires changes to proceed.

I have access to an up-to-date copy of the isa book and can review these for technical correctness. I'm less familiar with style issues.

I have checked the bitfields, encodings, and field names.  They are all correct.

Some comments below, especially on the xSYNC instructions. Nothing too serious, but they do have side effects of one form or another. Typically a compiler wouldn't generate them unless as part of a larger hard-coded, unsplittable block, but if they are present, they ought to be correct.



================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrFormats.td:52
+
+  let Inst{23-20} = op2;
+  let Inst{19-16} = op1;
----------------
These fields are reverse order in big endian versions of the core. Is the ESP32 implementation of Xtensa always little endian?

It would be nice to handle both variants, but that isn't a block for this patch.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:101
+  let s = 0x0;
+  let hasSideEffects = 1;
+}
----------------
This is quite a bit more conservative than necessary. MEMW only serializes memory operations, it is fine to move arithmetic ops across it.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:108
+  let s = 0x0;
+  let t = 0xd;
+}
----------------
This instruction is a superset of MEMW and serializes all externally visible operations.

hasSideEffects is necessary on this one (at least in a base ISA implementation).


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:115
+
+def DSYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "dsync", []> {
----------------
DSYNC is the least intrusive processor-state sync instruction, used to serialize loads and stores against WSR and XSR instructions that may update the page table. 

It could be modeled quite similarly to MEMW, but with a more complete memory-barrier model, it should include side effects.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:122
+
+def ISYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "isync", []> {
----------------
ISYNC synchronizes the instruction fetch unit against updates to the icache. Needs hasSideEffects set.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:129
+
+def RSYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "rsync", []> {
----------------
This synchronizes reading register fields out of instructions against WSR and XSR modifying the register window. Needs hasSideEffects set.


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:136
+
+def ESYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "esync", []> {
----------------
This is quite similar to RSYNC, but synchronizes the exact read from the register window, rather than the fields out of the instruction. Needs hasSideEffects set.


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

https://reviews.llvm.org/D64830