[PATCH] D38537: [mips] Implement .set dspr2 directive

[Simon Dardis via Phabricator via llvm-commits]

sdardis added inline comments.


================
Comment at: test/MC/Mips/set-nodsp.s:1
-# RUN: not llvm-mc %s -mcpu=mips32 -mattr=+dsp -triple mips-unknown-linux 2>%t1
+# RUN: not llvm-mc %s -mcpu=mips32 -mattr=+dsp -mattr=+dspr2 -triple mips-unknown-linux 2>%t1
 # RUN: FileCheck %s < %t1
----------------
petarj wrote:
> -mattr=+dsp seems redundant in presence of -mattr=+dspr2
> 
> Furthermore, DSP extension requires Rev2 at least, so the test should probable have -mcpu=mips32r2
> 
> GCC has a warning like:
> 
> "//Warning: the `dsp' extension requires MIPS32 revision 2 or greater'//"
> 
> It seems that LLVM does not do that. Maybe it should?
> -mattr=+dsp seems redundant in presence of -mattr=+dspr2

Well spotted, though the test would have to be altered to test for the differences between dsp / dspr2.

> GCC has a warning like:

> "Warning: the `dsp' extension requires MIPS32 revision 2 or greater'"

> It seems that LLVM does not do that. Maybe it should?

I believe we should. I think we should be stricter but binutils already accepts code that combination of options so we should at least be as compatible / informative.

Some quick testing shows that gas 2.24 only warns on the first instance of a particular error in the test cases I've tried for mixing directives to change the ISA revision.

I'm leaning towards giving warnings where an ASE is enabled that requires a higher ISA revision by an assembler directive at every occurrence, along with a warning for incoherent command-line options, though that may be a bit verbose. Thoughts?

Adding warnings for incompatible ASE/ISA revisions should be submitted as a separate patch.


https://reviews.llvm.org/D38537