[Bug middle-end/65449] New: -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[Bug middle-end/65449] New: -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[ma.jiang at zte dot com.cn]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65449

            Bug ID: 65449
           Summary: -fstrict-volatile-bitfields affects volatile pointer
                    dereference and produce wrong codes
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ma.jiang at zte dot com.cn

Hi,all.
   It seems that  -fstrict-volatile-bitfields  can affect volatile pointer
dereference. However, the gcc manual said this option should only affect
accesse to bit-fields or structure fields.
Compiling the test case: 
char mt[20];
void main()
{
    void *mm=&(mt[1]);
  *((volatile int *)mm)=4;
}
 with -O2 -mstrict-align -fstrict-volatile-bitfields on PPC. We can see that 
"*((volatile int *)mm)=4 " is done by a single stw. Beware that -mstrict-align
means  a non-aligned memory access is disallowed, and &(mt[1]) is obviously not
a address aligned to 4-bytes boundary.  The compiler should have no reasons to
produce a unaligned stw when mstric-align is on.

Further more,compiling with -O2 -mstrict-align -fno-strict-volatile-bitfields,
the compiler will produce four lbz/stb pairs for "*((volatile int *)mm)=4;".
This is  ridiculous as the C standard does not require the read, and surely no
performance benefits could grain from these lbz.

[Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65449

--- Comment #1 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Hi Richard,

the invalid/different code for -O2 -fstrict-volatile-bitfields will be
fixed with my proposed patch, because the misalignedness of mm should
be visible at -O2 and prevent the strict_volatile bitfields path to be
entered.

Could you give your OK to the latest version?
see https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00817.html

Thanks
Bernd.

[Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[ma.jiang at zte dot com.cn]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65449

--- Comment #2 from ma.jiang at zte dot com.cn ---
(In reply to Bernd Edlinger from comment #1)
> Hi Richard,
> 
> the invalid/different code for -O2 -fstrict-volatile-bitfields will be
> fixed with my proposed patch, because the misalignedness of mm should
> be visible at -O2 and prevent the strict_volatile bitfields path to be
> entered.
> 
> Could you give your OK to the latest version?
> see https://gcc.gnu.org/ml/gcc-patches/2015-03/msg00817.html
> 
> Thanks
> Bernd.

Hi Bernd,
   Your patch do fix the unaligned stw problem. But,there are still 4 lbz
instructions for  "*((volatile int *)mm)=4;" after your fix. I thought they
were caused by the -fstrict-volatile-bitfields originally.After a more careful
check, I find they are caused by " temp = force_reg (mode, op0);" in
store_fixed_bit_field_1. The "*((int *)mm)=4;" produce  no lbz instructions,
but still produce useless load when doing rtl expand.

(insn 7 6 8 2 (set (reg:QI 157)
        (mem/c:QI (plus:SI (reg/f:SI 155)
                (const_int 1 [0x1])) [1 MEM[(int *)&mt + 1B]+0 S1 A8])) nt.c:5
489 {*movqi_internal}
     (nil))
These loads will be eliminated in fwprop1 as they are meaningless.However,
after adding volatile for the memory mm, the fwprop1 pass can not delete these
loads as volatile loads should not be removed.
  So, I think we should first get rid of the volatile flag from op0 before call
force_reg (mode, op0). I have tried to adding "rtx op1 = copy_rtx (op0);
MEM_VOLATILE_P(op1)=0;"  just before force_reg, and it do remove those lbz
instruction.

[Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[bernd.edlinger at hotmail dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65449

--- Comment #3 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
Yes, but that is not the fault of the strict volatile code path any more.

For bit-fields this redundant read is exactly what AAPCS demands:

"7.1.7.5 Volatile bit - fields preserving number and width of container
accesses

When a volatile bit-field is read, its container must be read exactly
once using the access width appropriate to the type of the container.

When a volatile bit-field is written, its container must be read exactly
once and written exactly once using the access width appropriate to the
type of the container. The two accesses are not atomic."


IMO, the problem is this:

In store_fixed_bit_field_1 we do not know if the access is on a
packed structure member where the extra read is not necessary,
or if we have a bit-field where the extra read would be mandatory,
even if the complete container is overwritten.


BTW: What happens in your example if you use -O0? Isn't there still an
unaligned stw access?  That's because you example is in a way invalid C.
You can't set an int* to an unaligned address, it must be at least
aligned to the GET_MODE_ALIGNMENT(SImode).

[Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes

[ma.jiang at zte dot com.cn]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65449

--- Comment #4 from ma.jiang at zte dot com.cn ---
Hi Bernd,
  Thanks for your apply.

(In reply to Bernd Edlinger from comment #3)
> Yes, but that is not the fault of the strict volatile code path any more.
//  Sure,I agree with you. The redundant read  is another problem. In fact, it
should be named as "volatile pointer dereference produce redundant read".

> For bit-fields this redundant read is exactly what AAPCS demands:
> 
//   Yes.I know that volatile bitfiled need the redundant read. That is why I
thought there were connections between the redundant read and
-fstrict-volatile-bitfields originally.

> IMO, the problem is this:
> 
> In store_fixed_bit_field_1 we do not know if the access is on a
> packed structure member where the extra read is not necessary,
> or if we have a bit-field where the extra read would be mandatory,
> even if the complete container is overwritten.
// I agree that the problem is caused by the store_fixed_bit_field_1.But,I
disgree with you about three things.First,the redundant read should not appear
if -fstrict-volatile-bitfields was off. Second, in store_fixed_bit_field_1 we
can distinguish pointer dereference from structure member
access(e.g.,MEM_EXPR(op0) will tell us whether op0 is a componet_ref or not, if
MEM_P(op0) is true). Third, as gcc manual said "-fstrict-volatile-bitfields
:This option should be used if accesses to volatile bit-fields (or other
structure fields, although the compiler usually honors those types anyway)
should use a single access of the width of the field’s type, aligned to a
natural alignment if possible.", store_fixed_bit_field_1 does not need to
distinguish bitfields from normal structure member.

> 
> BTW: What happens in your example if you use -O0? Isn't there still an
> unaligned stw access?  That's because you example is in a way invalid C.
> You can't set an int* to an unaligned address, it must be at least
> aligned to the GET_MODE_ALIGNMENT(SImode).
//Yes, I know what you mean. Split access is a auxiliary fuction provided by
compiler with the help of data analysis.As -O0 turn off all analysis , an
unaligned stw is acceptable. And ,BTW, the C standard said nothing about
unaligned access as I know. Could you show me something about it?
>From gcc-bugs-return-480900-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Fri Mar 20 02:51:51 2015
Return-Path: <gcc-bugs-return-480900-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 90059 invoked by alias); 20 Mar 2015 02:51:50 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 90022 invoked by uid 48); 20 Mar 2015 02:51:47 -0000
From: "hubicka at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug ipa/65483] New: bzip2 bsR/bsW should be auto-inlined
Date: Fri, 20 Mar 2015 05:32:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: new
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: ipa
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: hubicka at gcc dot gnu.org
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_id short_desc product version bug_status bug_severity priority component assigned_to reporter
Message-ID: <bug-65483-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-03/txt/msg02044.txt.bz2
Content-length: 3978

https://gcc.gnu.org/bugzilla/show_bug.cgi?ide483

            Bug ID: 65483
           Summary: bzip2 bsR/bsW should be auto-inlined
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hubicka at gcc dot gnu.org

bzip2 contains:
INLINE UInt32 bsR ( Int32 n )
{
   UInt32 v;
   bsNEEDR ( n );
   v = (bsBuff >> (bsLive-n)) & ((1 << n)-1);
   bsLive -= n;
   return v;
}

and

INLNE void bsW ( Int32 n, UInt32 v )
{
   bsNEEDW ( n );
   bsBuff |= (v << (32 - bsLive - n));
   bsLive += n;
}

which should be inlined.  INLINE is however defined to nothing for SPEC.
The catch is that we instead inline fgetc/fputc into the functions here:

#define bsNEEDR(nz)                           \
{                                             \
   while (bsLive < nz) {                      \
      Int32 zzi = fgetc ( bsStream );         \
      if (zzi == EOF) compressedStreamEOF();  \
      bsBuff = (bsBuff << 8) | (zzi & 0xffL); \
      bsLive += 8;                            \
   }                                          \
}


/*---------------------------------------------*/
#define bsNEEDW(nz)                           \
{                                             \
   while (bsLive >= 8) {                      \
      fputc ( (UChar)(bsBuff >> 24),          \
               bsStream );                    \
      bsBuff <<= 8;                           \
      bsLive -= 8;                            \
      bytesOut++;                             \
   }                                          \
}

Considering spec_getc/285 with 33 size
 to be inlined into bsR/98 in unknown:-1
 Estimated badness is -21.814074, frequency 21.04.
    Badness calculation for bsR/98 -> spec_getc/285
      size growth 27, time 22 inline hints: cross_module big_speedup
      -10.907037: guessed profile. frequency 21.035000, count 0 caller count 0
time w/o inlining 1063.840001, time w inlining 769.350000 overall growth 0
(current) 0 (original)
      Adjusted by hints -21.814074
                Accounting size:20.00, time:304.69 on predicate:(true)
...
 Inlined into bsR which now has time 767 and size 55,net change of +27.

which makes it to reach inline-insns-auto limit.

bsR is estimated as:

Inline summary for bsR/98 inlinable
  self time:       559
  global time:     0
  self size:       28
  global size:     0
  min size:       0
  self stack:      0
  global stack:    0
    size:21.000000, time:304.328000, predicate:(true)
    size:3.000000, time:1.982000, predicate:(not inlined)
  calls:
    compressedStreamEOF/143 function not considered for inlining
      loop depth: 0 freq:   8 size: 1 time: 10 callee size:12 stack: 0
    spec_getc/153 function body not available
      loop depth: 1 freq:21035 size: 3 time: 12 callee size: 0 stack: 0

The spec_getc is implemented as:

int spec_getc (int fd) {
    int rc = 0;
    debug1(4,"spec_getc: %d = ", fd);
    if (fd > MAX_SPEC_FD) {
        fprintf(stderr, "spec_read: fd=%d, > MAX_SPEC_FD!\n", fd);
        exit (1);
    }
    if (spec_fd[fd].pos >= spec_fd[fd].len) {
        debug(4,"EOF\n");
        return EOF;
    }
    rc = spec_fd[fd].buf[spec_fd[fd].pos++];
    debug1(4,"%d\n", rc);
    return rc;
}

we however split out the error handling into spec_getc.part and get:

Inline summary for spec_getc/38 inlinable
  self time:       24
  global time:     0
  self size:       33
  global size:     0
  min size:       0
  self stack:      0
  global stack:    0
    size:20.000000, time:14.485000, predicate:(true)
    size:3.000000, time:1.998000, predicate:(not inlined)

which makes it quite good inline candidate especially because the call appears
within what we consider an internal loop of bsR.

Apparently clang gets lucky here because it inlines more at copmile time and
spec_getc is housed in different translation unit.