This is an archive of the discontinued LLVM Phabricator instance.

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:

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?
Are you planning to support .module arch=octeon?
Are you planning to support clang with -march=octeon?
Are you planning to support the octeon arch e_flags in the object writer?
Are octeon+, octeon2, octeon3 also going to be included
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

atanasyan added a subscriber: atanasyan.Mar 30 2016, 7:06 AM

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 ?

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.

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

Comments addressed.

Herald added a subscriber: sdardis. · View Herald TranscriptApr 11 2016, 8:56 AM

LGTM

This revision is now accepted and ready to land.Apr 12 2016, 1:56 AM

Closed by commit rL266081: [mips] add assembler support for .set arch=octeon (authored by petarj). · Explain WhyApr 12 2016, 8:33 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents

Path

Size

lib/

Target/

Mips/

AsmParser/

MipsAsmParser.cpp

2 lines

test/

MC/

Mips/

set-arch.s

6 lines

Diff 53234

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

Show First 20 Lines • Show All 5,289 Lines • ▼ Show 20 Lines	StringRef ArchFeatureName =
.Case("mips32r3", "mips32r3")		.Case("mips32r3", "mips32r3")
.Case("mips32r5", "mips32r5")		.Case("mips32r5", "mips32r5")
.Case("mips32r6", "mips32r6")		.Case("mips32r6", "mips32r6")
.Case("mips64", "mips64")		.Case("mips64", "mips64")
.Case("mips64r2", "mips64r2")		.Case("mips64r2", "mips64r2")
.Case("mips64r3", "mips64r3")		.Case("mips64r3", "mips64r3")
.Case("mips64r5", "mips64r5")		.Case("mips64r5", "mips64r5")
.Case("mips64r6", "mips64r6")		.Case("mips64r6", "mips64r6")
.Case("cnmips", "cnmips")		.Case("octeon", "cnmips")
.Case("r4000", "mips3") // This is an implementation of Mips3.		.Case("r4000", "mips3") // This is an implementation of Mips3.
		petarjUnsubmitted Done Reply Inline Actions 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? petarj: Since the goal of this change is only to support ".set arch=octeon" in inline assembly, I am…
		mpfUnsubmitted Not Done Reply Inline Actions 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. mpf: In case it was not clear I was not objecting to this patch. I'm merely concerned that the…
		petarjUnsubmitted Not Done Reply Inline Actions I agree, that's another/related issue with the integrated assembler that should be resolved. petarj: I agree, that's another/related issue with the integrated assembler that should be resolved.
		dsandersUnsubmitted Done Reply Inline Actions 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. dsanders: > Having said this, shouldn't we delete the line above, since I do not think that ".set…
.Default("");		.Default("");

if (ArchFeatureName.empty())		if (ArchFeatureName.empty())
return reportParseError("unsupported architecture");		return reportParseError("unsupported architecture");

selectArch(ArchFeatureName);		selectArch(ArchFeatureName);
getTargetStreamer().emitDirectiveSetArch(Arch);		getTargetStreamer().emitDirectiveSetArch(Arch);
return false;		return false;
▲ Show 20 Lines • Show All 1,046 Lines • Show Last 20 Lines

test/MC/Mips/set-arch.s

Show All 30 Lines	# RUN: FileCheck %s
.set arch=mips64		.set arch=mips64
.set arch=mips64r3		.set arch=mips64r3
drotr32 $1, $14, 15		drotr32 $1, $14, 15
.set arch=mips64		.set arch=mips64
.set arch=mips64r5		.set arch=mips64r5
drotr32 $1, $14, 15		drotr32 $1, $14, 15
.set arch=mips64r6		.set arch=mips64r6
mod $2, $4, $6		mod $2, $4, $6
.set arch=cnmips		.set arch=octeon
		baddu $9, $6, $7
		dsandersUnsubmitted Done Reply Inline Actions 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 dsanders: Could you add an Octeon specific instruction after this line and add a check for it below? It…
.set arch=r4000		.set arch=r4000
dadd $2, $2, $2		dadd $2, $2, $2

# CHECK: .set arch=mips1		# CHECK: .set arch=mips1
# CHECK: add $2, $2, $2		# CHECK: add $2, $2, $2
# CHECK: .set arch=mips2		# CHECK: .set arch=mips2
# CHECK: ll $2, -2($2)		# CHECK: ll $2, -2($2)
# CHECK: .set arch=mips3		# CHECK: .set arch=mips3
Show All 9 Lines
# CHECK: .set arch=mips32r6		# CHECK: .set arch=mips32r6
# CHECK: mod $2, $4, $6		# CHECK: mod $2, $4, $6
# CHECK: .set arch=mips64		# CHECK: .set arch=mips64
# CHECK: daddi $2, $2, 10		# CHECK: daddi $2, $2, 10
# CHECK: .set arch=mips64r2		# CHECK: .set arch=mips64r2
# CHECK: drotr32 $1, $14, 15		# CHECK: drotr32 $1, $14, 15
# CHECK: .set arch=mips64r6		# CHECK: .set arch=mips64r6
# CHECK: mod $2, $4, $6		# CHECK: mod $2, $4, $6
# CHECK: .set arch=cnmips		# CHECK: .set arch=octeon
		# CHECK: baddu $9, $6, $7
# CHECK: .set arch=r4000		# CHECK: .set arch=r4000
# CHECK: dadd $2, $2, $2		# CHECK: dadd $2, $2, $2

This is an archive of the discontinued LLVM Phabricator instance.

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

Details

Diff Detail

Event Timeline

Revision Contents

Diff 53234

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

test/MC/Mips/set-arch.s

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