msp430 port

[Mike Stump]

On Aug 19, 2013, at 1:21 PM, Jeff Law <law@redhat.com> wrote:
> On 07/30/2013 09:24 PM, DJ Delorie wrote:
>> [nickc added for comments about the bits he wrote]
>> 
>>> ... define these as
>>> 
>>> (define_predicate "msp_general_operand"
>>>   (match_code "mem,reg,subreg,const_int,const,symbol_ref,label_ref"
>>> {
>>>   int save_volatile_ok = volatile_ok;
>>>   volatile_ok = 1;
>>>   int ret = general_operand (op, mode);
>>>   volatile_ok = save_volatile_ok;
>>>   return ret;
>>> })
>>> 
>>> That said, why do they exist?
>> 
>> Because gcc refuses to optimize operations with volatile memory
>> references, even when the target has opcodes that follow the volatile
>> memory rules.  "set bit in memory" for example.  I've had to do
>> something like this for every port I've written, for the same reason,
>> despite arguing against gcc's pedantry about volatile memory accesses.
> I'd say it's not as simple as you make it out to be.  You can't blindly combine operations on volatile memory.  ie, the programmer may have written them as separate statements and if they're volatile you should leave them alone.  With this change a series of statements involving volatiles might be combined into a single insn.  That's not good.
> 
> So while you will get better optimization in the presence of volatile, you'll also eventually get a mis-optimization of a volatile reference. And debugging it will be an absolute nightmare because whether or not it causes a visible problem will be dependent on the state of some hardware widget.
> 
> I'll note that *NO* ports in the official GCC sources do this and that's because it's wrong.  Unfortunately, I missed this when looking at the port or I would have called it out earlier.  What you may have done for ports that are still in private trees is not pertinent.

First, some background for those that are new around here (you're not!):

http://gcc.gnu.org/ml/gcc-bugs/1999-12n/msg00762.html

http://gcc.gnu.org/ml/gcc/2005-11/msg00990.html

http://gcc.gnu.org/ml/gcc/2000-01/msg00400.html

http://gcc.gnu.org/ml/gcc-bugs/2000-06/msg00390.html


I disagree in part with you.  The compiler is perfectly free to optimize volatile in many different ways.  That we don't, is a bug that can be fixed, but that no one has yet bothered to do much about, other than sneaking around in the corners.  I, like you, don't favor the sneaking around in the corners.  I think the right approach is for people to flip on the optimization once the port is clean wrt volatile and the the back end bugs that arise from it that the port maintainer deems are reasonably under control (a port only decision).  Long term, we should define exactly what the ports must do (and not do), so that we can default on the optimizations.  All new ports should just be required to be correct wrt these requirements.

An example of what can be optimized:

volatile int a;

int main() {
	int b;
	b = a;
	++b;
	a = b;
}

Some machines have suitable instructions to do effectively an atomic add &a, 1.  This doesn't violate the spirit of the language standard or the sensibilities of people that write volatile code.  Of course, such code now would seem positivity quaint given <atomic> (aka lib atomic) type things.

I do understand that there are conditions that need to be checked for the code to be flawless, and that absent those checks, it won't be.  I just don't agree that dodging the issue for another 13 years is the best path forward.

So, re-cap, we aren't talking about doing anything blindly.  Separate statements can be combined.  Combining them is good.  One can combine things incorrectly, and we are not arguing that case.  Only the things that are correct, see above for one example.

[ glancing at trunk ]

Oh, cute, create_fixed_operand.  :-)  Someone has attacked the issue in a sideways fashion.  Nice start, though, doesn't get at the heart of the problem.

More constructive if for those that know of conditions that need to be checked, that are not, would instead add the checks, then, trivially, we can just turn it on and be done with it.  Slight less useful, but still productive, would be list give examples of codes that do the wrong thing.  For me personally, I know of no bugs.  I think the port should  experiment with just flipping the big switch, and because I didn't provide a link or a patch last time, let me include it now:

diff --git a/gcc/recog.c b/gcc/recog.c
index 0defd7d..a93eacd 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -1012,7 +1012,7 @@ general_operand (rtx op, enum machine_mode mode)
     {
       rtx y = XEXP (op, 0);
 
-      if (! volatile_ok && MEM_VOLATILE_P (op))
+      if (! TARGET_VOLATILE_CORRECT && ! volatile_ok && MEM_VOLATILE_P (op))
        return 0;
 
       /* Use the mem's mode, since it will be reloaded thus.  */


So that others know exactly what I'm thinking about.  Targets that fix their port to be clean with respect to volatile (you must not read or write in a manner other than is directly allowed if volatile is set) can turn it one and go on with life.

So, does anyone have a concrete example of what will not work on the msp430 port, if this is done?