This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by spetrovic on Mar 29 2016, 6:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

spetrovic updated this revision to Diff 51882.Mar 29 2016, 6:44 AM
spetrovic retitled this revision from to [Mips] add assembler support for .set arch=octeon.
spetrovic updated this object.
spetrovic added reviewers: dsanders, petarj.
spetrovic set the repository for this revision to rL LLVM.
spetrovic added subscribers: llvm-commits, rankov.
mpf added a subscriber: mpf.Mar 29 2016, 7:21 AM

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

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 added inline comments.Apr 1 2016, 5:35 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
5299

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 added inline comments.Apr 1 2016, 6:14 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
5299

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 added inline comments.Apr 1 2016, 9:33 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
5299

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

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

petarj edited edge metadata.Apr 7 2016, 11:28 AM

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 edited edge metadata.Apr 8 2016, 7:07 AM

LGTM with the test change

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
5299

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

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 updated this revision to Diff 53234.Apr 11 2016, 8:56 AM
spetrovic edited edge metadata.
spetrovic marked 3 inline comments as done.

Comments addressed.

dsanders accepted this revision.Apr 12 2016, 1:56 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 12 2016, 1:56 AM
This revision was automatically updated to reflect the committed changes.