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

[Andrei Safronov via Phabricator via llvm-commits]

andreisfr marked 6 inline comments as done.
andreisfr added a comment.

@saugustine , thank you very much for your comments!



================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrFormats.td:52
+
+  let Inst{23-20} = op2;
+  let Inst{19-16} = op1;
----------------
saugustine wrote:
> 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.
Yes, the ESP32 implementation is always little endian, and I didn't observed any public available big endian implementations. So, initial idea was to implement just little endian at first and maybe implement support of the big endian in future.


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


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:108
+  let s = 0x0;
+  let t = 0xd;
+}
----------------
saugustine wrote:
> 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).
Corrected


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:115
+
+def DSYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "dsync", []> {
----------------
saugustine wrote:
> 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.
Corrected


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


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


================
Comment at: llvm/lib/Target/Xtensa/XtensaInstrInfo.td:136
+
+def ESYNC : RRR_Inst<0x00, 0x00, 0x00, (outs), (ins),
+                    "esync", []> {
----------------
saugustine wrote:
> 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.
Corrected


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

https://reviews.llvm.org/D64830