* [Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes
2015-03-17 9:28 [Bug middle-end/65449] New: -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes ma.jiang at zte dot com.cn
2015-03-18 12:12 ` [Bug middle-end/65449] " bernd.edlinger at hotmail dot de
@ 2015-03-19 10:03 ` ma.jiang at zte dot com.cn
2015-03-19 14:33 ` bernd.edlinger at hotmail dot de
2015-03-20 5:14 ` ma.jiang at zte dot com.cn
3 siblings, 0 replies; 5+ messages in thread
From: ma.jiang at zte dot com.cn @ 2015-03-19 10:03 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* [Bug middle-end/65449] -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes
2015-03-17 9:28 [Bug middle-end/65449] New: -fstrict-volatile-bitfields affects volatile pointer dereference and produce wrong codes ma.jiang at zte dot com.cn
` (2 preceding siblings ...)
2015-03-19 14:33 ` bernd.edlinger at hotmail dot de
@ 2015-03-20 5:14 ` ma.jiang at zte dot com.cn
3 siblings, 0 replies; 5+ messages in thread
From: ma.jiang at zte dot com.cn @ 2015-03-20 5:14 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 5+ messages in thread