| Submitter | Steve Ellcey |
|---|---|
| Date | July 17, 2012, 9:05 p.m. |
| Message ID | <4e207d4c-ed56-4c66-8bfd-e2a7b5bc9e97@EXCHHUB01.MIPS.com> |
| Download | mbox | patch |
| Permalink | /patch/171558/ |
| State | New |
| Headers | show |
Comments
"Steve Ellcey " <sellcey@mips.com> writes: > While working on my favorite mips option (-msynci) I noticed an odd thing. > If I compile with '-mips32 -mips16 -msynci' I got a warning about synci not > being supported but if I compiled with '-mips32r2 -mips16 -msynci' I did not > get a warning, even though -mips16 mode does not support synci. Furthermore > if I compiled a program that called __builtin___clear_cache with '-mips32r2 > -mips16 -msynci', the compiler would abort. The abort sounds like the bug here. It's deliberate that things like -msynci, -mbranch-likely, etc., are OK with -mips16. On the one hand, you could compile with -mips16 but have an __attribute__((nomips16)) function that could benefit from using SYNCI. On the other, you could compile without -mips16 but have an __attribute__((mips16)) function that needs to avoid SYNCI. -mips16 really just sets the default ISA mode for functions that don't specify one. That's why override_options hides mips16ness so early on, like you say. Richard
On Wed, 2012-07-18 at 18:30 +0100, Richard Sandiford wrote: > The abort sounds like the bug here. It's deliberate that things like > -msynci, -mbranch-likely, etc., are OK with -mips16. On the one hand, > you could compile with -mips16 but have an __attribute__((nomips16)) > function that could benefit from using SYNCI. On the other, you could > compile without -mips16 but have an __attribute__((mips16)) function > that needs to avoid SYNCI. OK, I think that makes sense. > -mips16 really just sets the default ISA mode for functions that don't > specify one. That's why override_options hides mips16ness so early on, > like you say. Ah, I didn't really understand why we were hiding the -mips16 setting, now I do. I will see if I can figure out why we abort. The clear_cache insn in mips.md looks a bit odd to me, there is the part that is executed when TARGET_SYNCI is true and then a part that is only executed if mips_cache_flush_func is defined. It looks like if mips_cache_flush_func is not defined then we do nothing and I was wondering if that is correct or not? Should mips_cache_flush_func being NULL be an error? I am not even sure if you can make it NULL given that it is given a default value in mips.opt. My test case is: void f() { int size = 40; char *memory = __builtin_alloca(size); __builtin___clear_cache(memory, memory + size); } And the abort with -mips32r2 -mips16 -msynci is: x.c: In function ‘f’: x.c:6:1: error: unrecognizable insn: } ^ (jump_insn 22 21 38 2 (set (pc) (if_then_else (eq (reg:SI 207) (reg/f:SI 196 [ D.1415 ])) (label_ref 33) (pc))) x.c:5 -1 (nil) -> 33) x.c:6:1: internal compiler error: in extract_insn, at recog.c:2129 Please submit a full bug report, with preprocessed source if appropriate. See <http://gcc.gnu.org/bugs.html> for instructions. If I can't figure out what is going on I will file a bug report. Steve Ellcey sellcey@mips.com
Patch
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 7356ce5..889cfb5 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -16212,6 +16212,14 @@ mips_option_override (void) target_flags &= ~MASK_SYNCI; } + /* ISA_HAS_SYNCI checks TARGET_MIPS16 but that was turned off at the + beginning of this function so we need to check mips_base_mips16. */ + if (TARGET_SYNCI && mips_base_mips16) + { + warning (0, "the \'mips16\' ASE does not support the synci instruction"); + target_flags &= ~MASK_SYNCI; + } + /* Only optimize PIC indirect calls if they are actually required. */ if (!TARGET_USE_GOT || !TARGET_EXPLICIT_RELOCS) target_flags &= ~MASK_RELAX_PIC_CALLS;
While working on my favorite mips option (-msynci) I noticed an odd thing. If I compile with '-mips32 -mips16 -msynci' I got a warning about synci not being supported but if I compiled with '-mips32r2 -mips16 -msynci' I did not get a warning, even though -mips16 mode does not support synci. Furthermore if I compiled a program that called __builtin___clear_cache with '-mips32r2 -mips16 -msynci', the compiler would abort. In mips.h we have: /* ISA includes synci, jr.hb and jalr.hb. */ #define ISA_HAS_SYNCI ((ISA_MIPS32R2 \ || ISA_MIPS64R2) \ && !TARGET_MIPS16) What I found was that in mips_option_override, where we check this macro to generate the warning we have this code at the front of the function: /* Process flags as though we were generating non-MIPS16 code. */ mips_base_mips16 = TARGET_MIPS16; target_flags &= ~MASK_MIPS16; Then later, we check ISA_HAS_SYNCI, but at that point TARGET_MIPS16 is always false because of the above lines. I looked at changing ISA_HAS_SYNCI to use target_flag_explicit but that seems like the wrong thing to do for the use of ISA_HAS_SYNCI in mips.md. Then I modified the if statement in mips_option_override but that resulted in the use of 'mips32r2 -mips16 -msynci' giving an odd warning message: warning: the ‘mips32r2’ architecture does not support the synci instruction But of course mips32r2 does support synci, it is -mips16 that does not. So I added a new if statement with an explicit check against mips_base_mips16 to give a better warning. OK to checkin? Steve Ellcey sellcey@mips.com 2012-07-17 Steve Ellcey <sellcey@mips.com> * config/mips/mips.c (mips_option_override): Fix check for -mips16 -msynci combination of flags.