------- Additional Comments From aoliva at gcc dot gnu dot org 2005-04-04 20:18 ------- Subject: Re: [Committed] PR c++/19199: Preserve COND_EXPR lvalueness in fold
On Apr 4, 2005, Roger Sayle <[EMAIL PROTECTED]> wrote: > My apologies yet again for not being more explicit about all of the > things that were wrong (and or I was unhappy with) with your proposed > solution. I'd hoped that I was clear in the comments in the bugzilla > thread, and that you'd appreciate the issues it addressed. I didn't. In fact, I only saw your responses in bugzilla early last week, since bugzilla e-mail I get goes to a folder that I don't read very often because of all the traffic in gcc-prs. > [1] The use of pedantic_lvalues to identify the non-C front-ends, > adversely affects code generation in the Java, Fortran and Ada > front-ends. Indeed. I'd misunderstood what pedantic_lvalues stood for, and thought it could be used to distinguish C++ from other languages, not C from other languages. My mistake. That's why it was never clear to me that you disliked its use, and why. > The use of COND_EXPRs as lvalues is unique to the > C++ front-end I don't know about that. Among all the languages supported by GCC, I'm only familiar with C, C++ and Java. > [3] Your patch is too invasive. Compared to the four-line counter > proposal that disables just the problematic class of transformations, > your much larger patch inherently contains a much larger risk. For > example, there is absolutely no need to modify the source code on the > "A >= 0 ? A : -A -> abs (A)" paths as these transformations could > never interfere with the lvalueness of an expression. Granted. OTOH, the patch faulted in trying to be overly consistent in potentially-broken code, as opposed to keeping potentially-broken code broken. > Additionally, once one of the longer term solutions proposed by Mark > or me is implemented, all of these workarounds will have to be undone/ > reverted. By only affecting a single clause, we avoid the potential > for leaving historic code bit-rotting in the tree. If you're convinced that's the only clause that triggers the problem, great. I wasn't. > [4] Despite your counter claims you approach *does* inhibit the ability > of the the tree-ssa optimizers to synthesize MIN_EXPR and MAX_EXPR > nodes. Indeed. I still had the behavior from an earlier version of the patch, in which cond_expr returned different values from maybe_lvalue_p depending on whether we were in gimple form or not. Sorry about that. > [5] For the immediate term, I don't think its worth worrying about > converting non-lvalues into lvalues, No worries here, all of my effort was toward preserving lvalueness. > And although, not serious enough to warrant a [6], it should be pointed > out that several of your recent patches have introduced regressions. Oh, like the patch in which I did check in a testcase after bootstrapping it on amd64-linux-gnu, although I didn't realize it still failed on i686-pc-linux-gnu, a host on which I don't bootstrap very often any more because my 32-bit hardware has become too slow? > Indeed, you've not yet reported that your patch has been sucessfully > bootstraped or regression tested on any target triple. Did I have to, before checking the patch in? I didn't think so. > Indeed, your > approach of posting a patch before completing the prerequisite testing > sprecified/stressed in contribute.html, on more than one occassion > recently resulted in the patch having to be rewritten/tweaked. And the need for rewriting/tweaking is exactly the reason why I choose to post patches early. Quite often, patches don't go in as they're posted, since maintainers often request additional minor tweaks. At about 24 hours per test cycle, and about as much for patch review, I don't think posting a patch proposal early would hurt anyone. In general, I'm asking for feedback on the approach, rather than on the exact patch spelling. If people find the patch to be good as is, great. Otherwise, sometimes I can still tweak the patch and get it into the next build&test cycle. And, more importantly, I like to describe the problem and the solution while it's still fresh in my mind. If I wait 1 or 2 days to post it, I won't be as precise in my description, and if questions arise, I may fail to answer correctly. > Indeed, as witnessed in "comment #17", I've already approved an > earlier version your patch once, only to later discover you were > wasting my time. If you find that reviewing patch proposals is wasting your time, I'm sorry. In general, I run a `make all; make check-gcc or make check-g++' on a previously-built tree before starting a full bootstrap and posting the patch, but this is not enough to catch all errors. In fact, a full bootstrap and test cycle isn't enough either, since we don't all use the same hosts. So, patches are going to be posted that need revision. Since most of my patches have minor revisions requested, not posting the patch for early feedback wastes a lot of my time. How do we find a balance? > As a middle-end maintainer, having been pinged by the release manager > that we/you weren't making sufficient progress towards addressing the > issues with your patch, Only days after that did I find you'd added notes to bugzilla, while an e-mail reply would have caught my attention immediately. > Perhaps, I should ask the counter question to your comment #21? > In what way do you feel that the committed patch isn't clearly > superior to your proposed solution? p.s. Thanks for spotting my > mistake of leaving a bogus comment above maybe_lvalue_p. I can't immediately convince myself that it covers all the potentially-faulty cases, and it tests for lvalueness after stripping NOP_EXPRs, which may prevent optimization in case the operand of the NOP_EXPR looks like an lvalue, but the NOP_EXPR itself is clearly an rvalue. It's easy enough to get the lvalue variable to take in_gimple_form into account, and we can move the test for the language name to the assignment to lvalue instead of using pedantic_lvalues, if you really think that's better than a langhook. Personally, I'd go with a lang-specific boolean that defaults to false, to tell whether a cond_expr/min_expr/max_expr can be an lvalue. I thought pedantic_lvalues was just that, but it's now obvious that I was mistaken. -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19199
