Hi,
Thanks Mei for the review comments on previous fix. I have incorporated these
comments and an updated patch is attached for review.
The rationale behind this fix is that, it is not correct to mark the
statement(i=i) as dead when the lhs is not propagatable. This patch addresses
this issue.
Thanks,
Shivaram
From: Rao, Shivarama [mailto:shivarama....@amd.com]
Sent: Saturday, September 24, 2011 8:31 AM
To: open64-devel@lists.sourceforge.net
Subject: Re: [Open64-devel] Code Review request for bug787 [WOPT]
Hi,
A revised patch and test case is attached for review. Can a gatekeeper please
review the same?
The problem is with the following sequence of code:
if(1)
{
asm("bswapl %0" : "=r" (val0) : "0" (val1));
res1 = val0;
}
val0 = res1;
asm("bswapl %0" : "=r" (val1) : "0" (val0));
For some reason, we disable the copy propagation for assembly input operands.
In the above sequence, the copy propagation results in "val0=val0" assignment
statement (at line 6) and gets deleted by DCE. Since val0 of first ASM
statement doesn't propagate to 2nd ASM statement, we get no definition for its
operand and hence the assertion.
The function Is_identity_assignment_removable now checks for copy propagation
properties of lhs and rhs to see if it can be deleted.
Thanks,
Shivaram
From: Gautam Chakrabarti [mailto:gautam.c...@yahoo.com]
Sent: Sunday, September 18, 2011 10:54 AM
To: Rao, Shivarama; Sun Chan
Cc: open64-devel@lists.sourceforge.net
Subject: Re: [Open64-devel] Code Review request for bug787 [WOPT]
Hi Shivarama,
I am also not sure why you are making this change. It's not clear what wrong
transformation the simplifier is doing here. It appears to me it should be okay
to replace __builtin_constant(expr) with 0 or 1. Is the simplifer doing
anything more than this? Otherwise it appears that should be correct, and the
reason behind the WOPT assertion might be something else?
Thanks,
Gautam
From: "Rao, Shivarama" <shivarama....@amd.com>
To: Sun Chan <sun.c...@gmail.com>
Cc: "open64-devel@lists.sourceforge.net" <open64-devel@lists.sourceforge.net>
Sent: Saturday, September 17, 2011 8:44 PM
Subject: Re: [Open64-devel] Code Review request for bug787 [WOPT]
Hi Sun,
Thanks for the comments. I will further look into this and get back.
The bug fix 14470 has two parts. If the expression is found constant, the
expression is replaced with constant 1. If the expression is not found to be a
constant expression, it is replaced with a constant ZERO. While I agree with
the simplification of constant expressions (with CONSTANT 1), I am not sure,
if the 2nd simplification (non-constant expression) will be guaranteed not to
remove any useful expressions/definitions. I have removed the 2nd part in the
fix.
Regards,
Shivaram
From: Sun Chan [mailto:sun.c...@gmail.com]
Sent: Sunday, September 18, 2011 4:01 AM
To: Rao, Shivarama
Cc: open64-devel@lists.sourceforge.net
Subject: Re: [Open64-devel] Code Review request for bug787 [WOPT]
I don't fully understand why you remove the code in simplifier. Your rationale
doesn't seem logical. On top of that, the comment in the "removed" code
indicates a certain bug, meanging, if removed, that bug will come back. There
is nothing wrong with replacing a const bool expression with 0 or 1. I doubt
you are really fixing the problem with this fix
Sun
On Fri, Sep 16, 2011 at 6:11 AM, Rao, Shivarama
<shivarama....@amd.com<mailto:shivarama....@amd.com>> wrote:
Hi,
Could a gatekeeper please review the fix done for bug 787?
The problem is with the simplification of INTRN_CONSTANT_P intrinsic
(buitin_constant_p operand). The SIMPNODE_SimplifyIntrinsic function replaces
the INTRN_CONSTANT_P intrinsic with a constant operand 1 when found that all
operands are constants and with a 0 when they are not constants. When replacing
the intrinsic function, the current results are deleted.
In the current test case, the current result happen to be the input for
ASM_STMT (inlined assembly statements). SPRE_rename_var throws assertion for
zero_version_cr for ASM_STMT operands since their definitions are deleted.
Since the zeroing of INTRN_CONSTANT_P is done in lower_expr function while
lowering to CG, I feel it is better to remove the zeroing at
SIMPNODE_SimplifyIntrinsic function and the change is done that way.
The patch file and a simplified testcase is attached. The assertion can be
reproduced with the debug build compiler.
Regards,
Shivaram
------------------------------------------------------------------------------
BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA
http://p.sf.net/sfu/rim-devcon-copy2
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net<mailto:Open64-devel@lists.sourceforge.net>
https://lists.sourceforge.net/lists/listinfo/open64-devel
------------------------------------------------------------------------------
BlackBerry® DevCon Americas, Oct. 18-20, San Francisco, CA
http://p.sf.net/sfu/rim-devcon-copy2
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net<mailto:Open64-devel@lists.sourceforge.net>
https://lists.sourceforge.net/lists/listinfo/open64-devel
Index: be/opt/opt_dce.cxx
===================================================================
--- be/opt/opt_dce.cxx (revision 3747)
+++ be/opt/opt_dce.cxx (working copy)
@@ -2149,7 +2149,8 @@
if (OPERATOR_is_scalar_store (opr) &&
Enable_identity_removal() &&
- stmt->Is_identity_assignment_removable()) // if COPYPROP assumes the
stmt is deleted
+ (stmt->Is_identity_assignment_removable() && // if COPYPROP assumes the
stmt is deleted
+ !(stmt->Lhs()->Flags() & CF_DONT_PROP)))
return FALSE;
// statements with zero-version chi nodes are required
@@ -3262,7 +3263,9 @@
if (OPERATOR_is_scalar_store (opr) &&
Enable_identity_removal() &&
- stmt->Is_identity_assignment_removable()) {
+ (stmt->Is_identity_assignment_removable() &&
+ !(stmt->Lhs()->Flags() & CF_DONT_PROP))) {
// process the rhs expression, if any
CODEREP *rhs = stmt->Rhs();
if ( rhs != NULL ) {
------------------------------------------------------------------------------
All the data continuously generated in your IT infrastructure contains a
definitive record of customers, application performance, security
threats, fraudulent activity and more. Splunk takes this data and makes
sense of it. Business sense. IT sense. Common sense.
http://p.sf.net/sfu/splunk-d2dcopy1
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
