Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2]
(Sorry, repost - I'd meant to cc Mike and Rainer into the conversation, but forgot to add them). On 3 October 2011 13:53, David Gilbert wrote: > On 30 September 2011 14:21, Ramana Radhakrishnan > wrote: >> Hi Dave, >> >> >> The nit-picky bit - There are still a number of formatting issues with >> your patch . Could you run your patch through >> contrib/check_GNU_style.sh and correct these. These are typically >> around problems with the number of spaces between a full stop and the >> end of comment, lines with trailing whitespaces and a few lines with >> number of characters > 80. Thanks. > > Oops - sorry about those; I'll run it through the check script and nail them. > >>>@@ -23590,82 +23637,142 @@ arm_output_sync_loop (emit_f emit, >>> >>>+ else >>>+ { >>>+ /* Silence false potentially unused warning */ >>>+ required_value_lo = NULL; >>>+ required_value_hi = NULL; >>>+ } >>> >> >> s/NULL/NULL_RTX in a number of places in arm.c > > OK. > >>>+ /* The restrictions on target registers in ARM mode are that the two >>>+ registers are consecutive and the first one is even; Thumb is >>>+ actually more flexible, but DI should give us this anyway. >>>+ Note that the 1st register always gets the lowest word in memory. */ >>>+ gcc_assert ((REGNO (value) & 1) == 0); >>>+ operands[2] = gen_rtx_REG (SImode, REGNO (value) + 1); >>>+ operands[3] = memory; >>>+ arm_output_asm_insn (emit, 0, operands, "strexd%s\t%%0, %%1, %%2, >>>%%C3", >>>+ cc); >>>+ } >>> >> >> The restriction is actually mandatory for ARM state only and thus I'm fine >> with this assertion being true only in ARM state. > > OK, I can make the assert only for thumb mode; but I thought the simpler > logic was better and should hold true anyway because of DI mode allocation. > >> I don't like duplicating the tests from gcc.dg into gcc.target/arm. >> If you wanted to check for assembler output specific to a target you could >> add your own conditions to the test in gcc.dg and conditionalize that on >> target arm_eabi >> >> Something like : >> >> { dg-final { scan-assembler "ldrexd\t"} {target arm_eabi}} } . >> >> I would like a testsuite maintainer to comment on the testsuite >> infrastructure >> bits as well but I have a few comments below . > > As discussed, I don't like the dupes either - the problem is that we > have 3 tests > with identical code but different dg annotation: > > 1) Build & run and check that the sync behaves correctly - using whatever > compile flags you happen to have. (gcc.dg version) > 2) Build and check assembler for use of ldrexd - compiled with armv6k flags > 3) Build and check assembler doesn't use ldrexd - compiled with armv5 flags > > Because (2) and (3) include different dg-add-options lines I don't see > how I can combine them. > > The suggestion that I'm OK with is to #include the gcc.dg one in the > gcc.arm one. > +# Return 1 if the target supports atomic operations on "long long" and can actually >>>+# execute them >>>+# So far only put checks in for ARM, others may want to add their own >>>+proc check_effective_target_sync_longlong { } { >>>+ return [check_runtime sync_longlong_runtime { >>>+ #include >>>+ int main() >>>+ { >>>+ long long l1; >>>+ >>>+ if (sizeof(long long)!=8) >> >> Space between ')' and ! as well as '=' and 8 >> >>>+ exit(1); >>>+ >>>+ #ifdef __arm__ >> >> Why is this checking only for ARM state ? We could have ldrexd in T2 as >> well ? > > Because __arm__ gets defined for either thumb or arm mode; in thumb mode > we just get __thumb__ (and __thumb2__) defined as well. > >> Otherwise the functionality looks good to me. Can you confirm that >> this has survived a testrun for v7-a thumb2 and v7-a arm state ? > > Yes it did. I'll give it another whirl later today after I go and fix > the formatting niggles and mvoe the test. > > Thanks for the review. > > Dave >
Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2]
On 30 September 2011 14:21, Ramana Radhakrishnan wrote: > Hi Dave, > > > The nit-picky bit - There are still a number of formatting issues with > your patch . Could you run your patch through > contrib/check_GNU_style.sh and correct these. These are typically > around problems with the number of spaces between a full stop and the > end of comment, lines with trailing whitespaces and a few lines with > number of characters > 80. Thanks. Oops - sorry about those; I'll run it through the check script and nail them. >>@@ -23590,82 +23637,142 @@ arm_output_sync_loop (emit_f emit, >> >>+ else >>+ { >>+ /* Silence false potentially unused warning */ >>+ required_value_lo = NULL; >>+ required_value_hi = NULL; >>+ } >> > > s/NULL/NULL_RTX in a number of places in arm.c OK. >>+ /* The restrictions on target registers in ARM mode are that the two >>+ registers are consecutive and the first one is even; Thumb is >>+ actually more flexible, but DI should give us this anyway. >>+ Note that the 1st register always gets the lowest word in memory. */ >>+ gcc_assert ((REGNO (value) & 1) == 0); >>+ operands[2] = gen_rtx_REG (SImode, REGNO (value) + 1); >>+ operands[3] = memory; >>+ arm_output_asm_insn (emit, 0, operands, "strexd%s\t%%0, %%1, %%2, >>%%C3", >>+ cc); >>+ } >> > > The restriction is actually mandatory for ARM state only and thus I'm fine > with this assertion being true only in ARM state. OK, I can make the assert only for thumb mode; but I thought the simpler logic was better and should hold true anyway because of DI mode allocation. > I don't like duplicating the tests from gcc.dg into gcc.target/arm. > If you wanted to check for assembler output specific to a target you could > add your own conditions to the test in gcc.dg and conditionalize that on > target arm_eabi > > Something like : > > { dg-final { scan-assembler "ldrexd\t"} {target arm_eabi}} } . > > I would like a testsuite maintainer to comment on the testsuite infrastructure > bits as well but I have a few comments below . As discussed, I don't like the dupes either - the problem is that we have 3 tests with identical code but different dg annotation: 1) Build & run and check that the sync behaves correctly - using whatever compile flags you happen to have. (gcc.dg version) 2) Build and check assembler for use of ldrexd - compiled with armv6k flags 3) Build and check assembler doesn't use ldrexd - compiled with armv5 flags Because (2) and (3) include different dg-add-options lines I don't see how I can combine them. The suggestion that I'm OK with is to #include the gcc.dg one in the gcc.arm one. >>> +# Return 1 if the target supports atomic operations on "long long" and can >>> actually >>+# execute them >>+# So far only put checks in for ARM, others may want to add their own >>+proc check_effective_target_sync_longlong { } { >>+ return [check_runtime sync_longlong_runtime { >>+ #include >>+ int main() >>+ { >>+ long long l1; >>+ >>+ if (sizeof(long long)!=8) > > Space between ')' and ! as well as '=' and 8 > >>+ exit(1); >>+ >>+ #ifdef __arm__ > > Why is this checking only for ARM state ? We could have ldrexd in T2 as > well ? Because __arm__ gets defined for either thumb or arm mode; in thumb mode we just get __thumb__ (and __thumb2__) defined as well. > Otherwise the functionality looks good to me. Can you confirm that > this has survived a testrun for v7-a thumb2 and v7-a arm state ? Yes it did. I'll give it another whirl later today after I go and fix the formatting niggles and mvoe the test. Thanks for the review. Dave
Re: [Patch 1/4] ARM 64 bit sync atomic operations [V2]
Hi Dave, The nit-picky bit - There are still a number of formatting issues with your patch . Could you run your patch through contrib/check_GNU_style.sh and correct these. These are typically around problems with the number of spaces between a full stop and the end of comment, lines with trailing whitespaces and a few lines with number of characters > 80. Thanks. >@@ -23590,82 +23637,142 @@ arm_output_sync_loop (emit_f emit, > >+ else >+ { >+/* Silence false potentially unused warning */ >+required_value_lo = NULL; >+required_value_hi = NULL; >+ } > s/NULL/NULL_RTX in a number of places in arm.c >@@ -23516,14 +23530,41 @@ arm_output_strex (emit_f emit, > rtx value, > rtx memory) > { >- const char *suffix = arm_ldrex_suffix (mode); >- rtx operands[3]; >+ rtx operands[4]; > > operands[0] = result; > operands[1] = value; >- operands[2] = memory; >- arm_output_asm_insn (emit, 0, operands, "strex%s%s\t%%0, %%1, %%C2", suffix, >- cc); >+ if (mode != DImode) >+{ >+ const char *suffix = arm_ldrex_suffix (mode); >+ operands[2] = memory; >+ arm_output_asm_insn (emit, 0, operands, "strex%s%s\t%%0, %%1, %%C2", >+suffix, cc); >+} >+ else >+{ >+ /* The restrictions on target registers in ARM mode are that the two >+ registers are consecutive and the first one is even; Thumb is >+ actually more flexible, but DI should give us this anyway. >+ Note that the 1st register always gets the lowest word in memory. */ >+ gcc_assert ((REGNO (value) & 1) == 0); >+ operands[2] = gen_rtx_REG (SImode, REGNO (value) + 1); >+ operands[3] = memory; >+ arm_output_asm_insn (emit, 0, operands, "strexd%s\t%%0, %%1, %%2, %%C3", >+ cc); >+} > The restriction is actually mandatory for ARM state only and thus I'm fine with this assertion being true only in ARM state. I don't like duplicating the tests from gcc.dg into gcc.target/arm. If you wanted to check for assembler output specific to a target you could add your own conditions to the test in gcc.dg and conditionalize that on target arm_eabi Something like : { dg-final { scan-assembler "ldrexd\t"} {target arm_eabi}} } . I would like a testsuite maintainer to comment on the testsuite infrastructure bits as well but I have a few comments below . >> +# Return 1 if the target supports atomic operations on "long long" and can >> actually >+# execute them >+# So far only put checks in for ARM, others may want to add their own >+proc check_effective_target_sync_longlong { } { >+return [check_runtime sync_longlong_runtime { >+ #include >+ int main() >+ { >+ long long l1; >+ >+ if (sizeof(long long)!=8) Space between ')' and ! as well as '=' and 8 >+exit(1); >+ >+ #ifdef __arm__ Why is this checking only for ARM state ? We could have ldrexd in T2 as well ? Otherwise the functionality looks good to me. Can you confirm that this has survived a testrun for v7-a thumb2 and v7-a arm state ? cheers Ramana