[PATCH] D14390: [mips] Expansion of LI.S and LI.D

[Simon Dardis via Phabricator via llvm-commits]

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

I missed something the first time around reviewing this. The usage of the .lit4 & .lit8 is conditional on the usage of the sdata section. For now, you can removed the .litX handling and just use .rodata in all cases. There's a longer explanation inline.

Thanks,
Simon



================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2736-2737
+static unsigned nextReg(unsigned Reg) {
+  if (MipsMCRegisterClasses[Mips::FGR32RegClassID].contains(Reg))
+    return Reg + 1;  
+  switch (Reg) {
----------------
This is almost correct. GAS will assemble "li.d $f31, 2.5" into "lui $at, 0x4004; mtc $at, $f0; mtc $zero, $f31".

You'll need to expand out the check or check for that specific case. 


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2771
+  case Mips::FP:   return Mips::RA;
+  case Mips::RA:   return Mips::ZERO; 
+  case Mips::D0:   return Mips::F1;
----------------
Stray space the end of the line.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2843-2849
+      MCSection *CS = getStreamer().getCurrentSectionOnly();
+      MCSection *Lit4Section = getContext().getELFSection(
+          ".lit4", ELF::SHT_PROGBITS,
+          ELF::SHF_ALLOC | ELF::SHF_WRITE | ELF::SHF_MIPS_GPREL);
+      getStreamer().SwitchSection(Lit4Section);
+      getStreamer().EmitIntValue(ImmOp32, 4);
+      getStreamer().SwitchSection(CS);
----------------
I missed this the first time around. The usage of .lit4 & .lit8 is permitted when: a) the small data section is in use, and b) the size of the constant is within the size threshold for the small data section.

If the small data section cannot be used, the constant is located within the .rodata section.

For the moment, just change this to always use the .rodata section, and add "FIXME: Enhance this expansion to use the .lit4 & .lit8 sections where appropriate."

This avoids having to modify MipsTargetObjectFile.cpp with arguably unrelated changes.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2968-2976
+    MCSection *CS = getStreamer().getCurrentSectionOnly();
+    MCSection *Lit8Section = getContext().getELFSection(
+        ".lit8", ELF::SHT_PROGBITS,
+        ELF::SHF_ALLOC | ELF::SHF_WRITE | ELF::SHF_MIPS_GPREL);
+
+    getStreamer().SwitchSection(Lit8Section);
+    getStreamer().EmitIntValue(HiImmOp64, 4);
----------------
See my comment about the .lit4 section.


================
Comment at: test/MC/Mips/macro-li.d.s:284
+# CHECK-MIPS64:                                           #   fixup A - offset: 0, value: %lit4(.lit8), kind: fixup_Mips_LITERAL
\ No newline at end of file

----------------
Please put a newline at the end of the file.


https://reviews.llvm.org/D14390