On Mon, Jan 16, 2017 at 03:09:35PM -0600, Aaron Sawdey wrote: > Tulio noted that glibc's strncmp test was failing. This turned out to > be the use of signed HOST_WIDE_INT for handling strncmp length. The > glibc test calls strncmp with length 2^64-1, presumably to provoke > exactly this type of bug. Fixing the issue required changing > select_block_compare_mode() and expand_block_compare() as well. > > The other change is if we must emit a runtime check for the 4k > crossing, then we might as well set base_align to 8 and emit the best > possible code.
Some nits... > --- gcc/config/rs6000/rs6000.c (revision 244503) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -19310,7 +19310,8 @@ > WORD_MODE_OK indicates using WORD_MODE is allowed, else SImode is > the largest allowable mode. */ > static machine_mode > -select_block_compare_mode (HOST_WIDE_INT offset, HOST_WIDE_INT bytes, > +select_block_compare_mode (unsigned HOST_WIDE_INT offset, > + unsigned HOST_WIDE_INT bytes, > HOST_WIDE_INT align, bool word_mode_ok) "align" should probably be unsigned as well? > @@ -19410,8 +19411,8 @@ > gcc_assert (GET_MODE (target) == SImode); > > /* Anything to move? */ > - HOST_WIDE_INT bytes = INTVAL (bytes_rtx); > - if (bytes <= 0) > + unsigned HOST_WIDE_INT bytes = INTVAL (bytes_rtx); > + if (bytes == 0) > return true; UINTVAL? Please check the rest of the patch for this, too. > /* We don't want to generate too much code. */ > if (ROUND_UP (bytes, load_mode_size) / load_mode_size > - > rs6000_block_compare_inline_limit) > + > (unsigned HOST_WIDE_INT)rs6000_block_compare_inline_limit) > return false; Space after cast operator. Why do you need a cast at all? It already is unsigned. > + /* -m32 -mpowerpc64 results in word_mode being DImode even > + though otherwise it is 32-bit. The length arg to strncmp > + is a size_t which will be the same size as pointers. */ > + rtx len_rtx; > + if (TARGET_64BIT) > + len_rtx = gen_reg_rtx(DImode); > + else > + len_rtx = gen_reg_rtx(SImode); Space before opening paren in function calls. > + while (bytes_to_compare > 0) > { > /* Compare sequence: > check each 8B with: ld/ld cmpd bne > + If equal, use rldicr/cmpb to check for zero byte. > cleanup code at end: > cmpb get byte that differs > cmpb look for zero byte Mixed spaces/tabs indent. > @@ -19919,37 +19968,45 @@ > } > } > > - int remain = bytes - cmp_bytes; > + /* Cases to handle. A and B are chunks of the two strings. > + 1: Not end of comparison: > + A != B: branch to cleanup code to compute result. > + A == B: check for 0 byte, next block if not found. > + 2: End of the inline comparison: > + A != B: branch to cleanup code to compute result. > + A == B: check for 0 byte, call strcmp/strncmp > + 3: compared requested N bytes: > + A == B: branch to result 0. > + A != B: cleanup code to compute result. */ And here. > @@ -19957,10 +20014,102 @@ > JUMP_LABEL (j) = dst_label; > LABEL_NUSES (dst_label) += 1; > > + if (remain > 0 || equality_compare_rest) > + { > + /* Generate a cmpb to test for a 0 byte and branch > + to final result if found. */ > + rtx cmpb_zero = gen_reg_rtx (word_mode); > + rtx lab_ref_fin = gen_rtx_LABEL_REF (VOIDmode, final_move_label); > + rtx condz = gen_reg_rtx (CCmode); > + rtx zero_reg = gen_reg_rtx (word_mode); > + if (word_mode == SImode) > + { > + emit_insn (gen_movsi (zero_reg, GEN_INT(0))); Space before (0). > + emit_insn (gen_cmpbsi3 (cmpb_zero, tmp_reg_src1, zero_reg)); > + if ( cmp_bytes < word_mode_size ) No spaces inside the parens. > + { /* Don't want to look at zero bytes past end. */ Put the comment on a separate line please. > + emit_insn (gen_movdi (zero_reg, GEN_INT(0))); > + emit_insn (gen_cmpbdi3 (cmpb_zero, tmp_reg_src1, zero_reg)); > + if ( cmp_bytes < word_mode_size ) > + { /* Don't want to look at zero bytes past end. */ Again and again and again :-) Looks good otherwise. Segher