Bug 53399 - "*ffs" pattern generates wrong code with BMI enabled
Summary: "*ffs" pattern generates wrong code with BMI enabled
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 4.8.0
Assignee: Not yet assigned to anyone
URL: http://gcc.gnu.org/ml/gcc-patches/201...
Keywords:
Depends on:
Blocks: 81763
  Show dependency treegraph
 
Reported: 2012-05-18 13:55 UTC by Yukhin Kirill
Modified: 2017-08-08 18:55 UTC (History)
4 users (show)

See Also:
Host:
Target: i?86-*-* x86_64-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-05-20 00:00:00


Attachments
testcase (243 bytes, text/plain)
2012-05-20 15:53 UTC, Yukhin Kirill
Details
Patch that separates flags setting tzcnt and bsf patterns (1.12 KB, patch)
2012-05-20 18:40 UTC, Uroš Bizjak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yukhin Kirill 2012-05-18 13:55:57 UTC
We have in GCC int. (__ffs description):
These functions return the index of the least significant 1-bit in a, or the value zero if a is zero.

and in i386.md:
(define_insn "*ffs<mode>_1"
  [(set (reg:CCZ FLAGS_REG)
        (compare:CCZ (match_operand:SWI48 1 "nonimmediate_operand" "rm")
                     (const_int 0)))
   (set (match_operand:SWI48 0 "register_operand" "=r")
        (ctz:SWI48 (match_dup 1)))]
  ""
{
  if (TARGET_BMI)
    return "tzcnt{<imodesuffix>}\t{%1, %0|%0, %1}";
  else 

This pattern works fine for bsf insn (although the result with zero input is undefined)
But for tzcnt with 0 as input we have (operand_size+1) as output.

That contradicts with GCC int, right?

It also seems to fail gcc.c-torture/execute/builtin-bitops-1.c
Comment 1 Yukhin Kirill 2012-05-18 13:58:22 UTC
(In reply to comment #0)
> It also seems to fail gcc.c-torture/execute/builtin-bitops-1.c
It fails on BMI-capable CPU
Comment 2 H.J. Lu 2012-05-18 14:28:40 UTC
This should be ok since the 0 input operand is ignored.
If not, please find out why.
Comment 3 Uroš Bizjak 2012-05-18 14:31:47 UTC
(In reply to comment #0)

> It also seems to fail gcc.c-torture/execute/builtin-bitops-1.c

Can you please isolate failing test?
Comment 4 Yukhin Kirill 2012-05-20 15:53:30 UTC
Created attachment 27449 [details]
testcase
Comment 5 Yukhin Kirill 2012-05-20 15:54:08 UTC
> 
> Can you please isolate failing test?

Sure, it is attached.
It works when compiled this way:
/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/xgcc -B/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/ 1.c -march=core2

And fails to run (on BMI*- capable HW) when compiled this way:
/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/xgcc -B/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/ 1.c -march=core-avx2
Comment 6 Yukhin Kirill 2012-05-20 15:54:38 UTC
> 
> Can you please isolate failing test?

Sure, it is attached.
It works when compiled this way:
/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/xgcc -B/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/ 1.c -march=core2

And fails to run (on BMI*- capable HW) when compiled this way:
/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/xgcc -B/export/home/kyukhin/gcc/build/build-x86_64-linux/gcc/ 1.c -march=core-avx2
Comment 7 H.J. Lu 2012-05-20 16:02:37 UTC
(In reply to comment #6)
> > 
> > Can you please isolate failing test?
> 
> Sure, it is attached.

Please show the generated __builtin_ffs assembly codes
compiled with and without BMI.   Also does

int
foo (int i)
{
  return __builtin_ffs (i);
}

work OK?
Comment 8 H.J. Lu 2012-05-20 16:18:25 UTC
BSF and tzcnt are different with zero input.
BSF sets ZF with zero input and tzcnt set CF
with zero input.
Comment 9 H.J. Lu 2012-05-20 16:24:13 UTC
We can't use tzcnt in the current ffs patterns.
We should add a CCC variant for ffs patterns with
tzcnt.
Comment 10 Uroš Bizjak 2012-05-20 18:40:24 UTC
Created attachment 27451 [details]
Patch that separates flags setting tzcnt and bsf patterns

Please test the attached patch. The patch checks CCCmode for TARGET_BMI in ffs patterns.
Comment 11 Yukhin Kirill 2012-05-21 11:02:07 UTC
> 
> Please test the attached patch. The patch checks CCCmode for TARGET_BMI in ffs
> patterns.

Hi Uros, seems your patch fixes the problem, here is piece of asm from testcase:
...
        movl    ints(%rip), %eax
        movl    %eax, %edx
        movl    $-1, %eax
        tzcntl  %edx, %ebx
        cmovc   %eax, %ebx
...
Comment 12 uros 2012-05-21 15:46:34 UTC
Author: uros
Date: Mon May 21 15:46:25 2012
New Revision: 187722

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=187722
Log:
	PR target/53399
	* config/i386/i386.md (ffs<mode>2): Generate CCCmode compare
	for TARGET_BMI.
	(ffssi2_no_cmove): Ditto.
	(*ffs<mode>_1): Remove insn pattern.
	(*tzcnt<mode>_1): New insn pattern.
	(*bsf<mode>1): Ditto.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
Comment 13 Uroš Bizjak 2012-05-21 15:54:43 UTC
Fixed for 4.8, not a regression on 4.7 branch.