[PATCH] D31628: Add Explanation of Mips Backend Relocation Principles

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

Some initial comments inlined, mostly focusing on accuracy. I'll take a second look later at 1 & 2.

One recurring nit is the usage of the word 'type'. In most of the contexts it is used here, it's actually referring to register class. The difference is that 'i32' is a type, but GPR32Opnd is a register class that can hold that type. I'd prefer not to confuse the reader by overloading the meaning of the word type.

Another nit is the usage of ILP32 mode and LP64 mode. For MIPS at least, we generally refer to these as N32 (our ILP32 ABI) and N64 (our LP64 ABI).



================
Comment at: Relocation.txt:4
+In LLVM, there are several elements of the llvm::ISD::NodeType enum that deal
+with adresses and/or relocations. These are defined in
+include/llvm/Target/TargetSelectionDAG.td, namely:
----------------
Nit: adresses -> addresses.


================
Comment at: Relocation.txt:41-42
+
+3. Table-gen "multiclass" is used to define patterns varying over load, add,
+zero, and type:
+
----------------
Depending on the knowledge level this document is targeted towards, this sentence is a bit poor and inaccurate.

   3. A TableGen multiclass pattern "MipsHiLoRelocs" is used to define a template pattern parameterised over the load, add operations, the zero register and register type:

Is somewhat better IMHO. If necessary, this document should reference the TableGen documentation for the specifics on multiclasses.


================
Comment at: Relocation.txt:60
+
+Here the instantiation of MipsHiLoRelocs in MipsInstrInfo.td is used by
+Mips32. The instantiation in Mips64InstrInfo.td is used by Mips64 in ILP32
----------------
I would move this sentence to just below the MIPS32 multiclass instantiation for clarity.


================
Comment at: Relocation.txt:61-62
+Here the instantiation of MipsHiLoRelocs in MipsInstrInfo.td is used by
+Mips32. The instantiation in Mips64InstrInfo.td is used by Mips64 in ILP32
+mode, as guarded by the predicate "SYM_32". A similar "multiclass" for Mips64
+in LP64 mode is also defined:
----------------
Nit: The SYM_32 predicate covers ILP32 mode and a submode of LP64 known as sym32, where symbols are assumed to be 32bits wide. I don't know if you want to cover that level of detail.


================
Comment at: Relocation.txt:84
+
+although it is only instantiated once:
+
----------------
Nit: It's actually instantiated twice. Once in Mips64InstrInfo.td and once in MicroMips64r6InstrInfo.td.


================
Comment at: Relocation.txt:89
+
+4. Instruction encoding is separated from types. For example:
+
----------------
This first sentence is inaccurate. The instruction definitions are multiply defined to cover the __different register classes__. In some cases, such as LW / LW64, this also accounts for the differences in the results of instruction execution. On MIPS32, "lw" loads a 32bit value from memory. On MIPS64, "lw" loads a 32 bit value from memory and sign extends the value to 64bits.


https://reviews.llvm.org/D31628