[Mips] add assembler support for .set arch=octeon

[spetrovic]

Summary

This patch enables assembler support for .set arch=octeon.

[mpf]

A few questions about the overall direction this core-specific support is going. I'm coming from the perspective of getting overall behavior of LLVM and GCC/binutils to be similar. I don't have a good understanding of current LLVM support though so please excuse any simplistic questions:

1. Is there any code to tell the assembler that octeon is a 64-bit arch and that it should switch to gp=64 automatically when seeing the .set octeon directive?
2. Are you planning to support .module arch=octeon?
3. Are you planning to support clang with -march=octeon?
4. Are you planning to support the octeon arch e_flags in the object writer?
5. Are octeon+, octeon2, octeon3 also going to be included
6. Are the octeon specific branch-on-bit instructions already supported? Either way if/when these are supported are they only enabled with the octeon arch. When these instructions are supported then (3) is quite important.

Thanks,
Matthew

[spetrovic]

I'm not planning to add more support for octeon. I just want to fix this issue while using -march=octeon and -fintegrated-as. The -march=octeon worked fine for me while using gnu assembler (without -fintegrated-as).

[petarj]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

5299 ↗

(On Diff #51882)

Since the goal of this change is only to support ".set arch=octeon" in inline assembly, I am fine with it.
Having said this, shouldn't we delete the line above, since I do not think that ".set arch=cnmips" is a valid option used anywhere?

[mpf]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

5299 ↗

(On Diff #51882)

In case it was not clear I was not objecting to this patch. I'm merely concerned that the support for octeon may end up inconsistent if nobody finishes the other parts of LLVM that would be affected. The most important part would be to sort out the integrated assembler to set E_MIPS_MACH_OCTEON if someone uses -march=octeon or .module=octeon otherwise an octeon elf will masquerade as a simple mips64r2.

[petarj]

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

5299 ↗

(On Diff #51882)

I agree, that's another/related issue with the integrated assembler that should be resolved.

[spetrovic]

Any more comments ? It seems to me that cnmips case is not used here. Maybe I should remove it ?

[petarj]

In D18548#393371, @spetrovic wrote:

Any more comments ? It seems to me that cnmips case is not used here. Maybe I should remove it ?

I believe it should be removed but this is not necessary for you to do it though.
I am fine with the change. @dsanders, @mpf, let me know if you do not want this change in for any reasons.

[dsanders]

LGTM with the test change

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

5299 ↗

(On Diff #51882)

Having said this, shouldn't we delete the line above, since I do not think that ".set arch=cnmips" is a valid option used anywhere?

I agree with removing the cnmips->cnmips mapping. I don't mind whether it's in this patch or separate.

The most important part would be to sort out the integrated assembler to set E_MIPS_MACH_OCTEON if someone uses -march=octeon or .module=octeon otherwise an octeon elf will masquerade as a simple mips64r2.

D18899 will fix this.

test/MC/Mips/set-arch.s

40 ↗

(On Diff #51882)

Could you add an Octeon specific instruction after this line and add a check for it below? It doesn't matter which one since we're just testing that the feature bits were updated correctly

[spetrovic]

Comments addressed.

[dsanders]

LGTM

llvm/trunk/lib/Target/Mips/AsmParser/MipsAsmParser.cpp

StringRef ArchFeatureName =
           .Case("mips32r3", "mips32r3")
           .Case("mips32r5", "mips32r5")
           .Case("mips32r6", "mips32r6")
           .Case("mips64", "mips64")
           .Case("mips64r2", "mips64r2")
           .Case("mips64r3", "mips64r3")
           .Case("mips64r5", "mips64r5")
           .Case("mips64r6", "mips64r6")
-          .Case("cnmips", "cnmips")
+          .Case("octeon", "cnmips")
           .Case("r4000", "mips3") // This is an implementation of Mips3.
           .Default("");
 
   if (ArchFeatureName.empty())
     return reportParseError("unsupported architecture");
 
   selectArch(ArchFeatureName);
   getTargetStreamer().emitDirectiveSetArch(Arch);

llvm/trunk/test/MC/Mips/set-arch.s

# RUN:   FileCheck %s
     .set arch=mips64
     .set arch=mips64r3
     drotr32     $1, $14, 15
     .set arch=mips64
     .set arch=mips64r5
     drotr32     $1, $14, 15
     .set arch=mips64r6
     mod         $2, $4, $6
-    .set arch=cnmips
+    .set arch=octeon
+    baddu $9, $6, $7
     .set arch=r4000
     dadd        $2, $2, $2
 
 # CHECK: .set arch=mips1
 # CHECK: add         $2, $2, $2
 # CHECK: .set arch=mips2
 # CHECK: ll          $2, -2($2)
 # CHECK: .set arch=mips3

 # CHECK: .set arch=mips32r6
 # CHECK: mod         $2, $4, $6
 # CHECK: .set arch=mips64
 # CHECK: daddi       $2, $2, 10
 # CHECK: .set arch=mips64r2
 # CHECK: drotr32     $1, $14, 15
 # CHECK: .set arch=mips64r6
 # CHECK: mod         $2, $4, $6
-# CHECK: .set arch=cnmips
+# CHECK: .set arch=octeon
+# CHECK: baddu $9, $6, $7
 # CHECK: .set arch=r4000
 # CHECK: dadd        $2, $2, $2