Re: [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 26, 2010, 18:31 UTC
Message-ID: <20100626183111.GB13070@burratino>
In-Reply-To: <4C250DD2.2030801@ramsay1.demon.co.uk>

Ramsay Jones wrote:

> Jonathan Nieder wrote:

Show 7 quoted lines

>> -#if defined(__i386__) || defined(__x86_64__)
>> +#if defined(__i386__) || defined(__x86_64__) || \
>> +	defined(_M_IX86) || defined(_M_X64)
>>    #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
>
> I looked at this, but decided not to make this change (while adding
> an item to my TODO list to investigate further).

Mmm, my only complaint is that leaving out this change makes the code appear to do something other than it does. Maybe a comment would help.

> After reading the large comment before line 57, and with a vague
> recollection of the mailing list discussion, I was left with the
> impression that this was aimed specifically at a quirk of the gcc
> code generator.

Sort of, but sort of not. Using volatile here is saying “I really want to do these stores in this order”. And that is probably the right thing to do for _any_ code generator on x86, unless it is very smart.

> In other words, maybe line 57 should read:
>
> #if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))

That would exclude icc etc so I’d prefer to avoid it without empirical evidence.

> It should probably be investigated at some point, but I don't think
> it's an urgent issue (and the msvc build will be no worse off ;-).

Right, I agree with this. :)  So for what it’s worth:

  Acked-by: Jonathan Nieder <jrnieder@gmail.com>

I am referring to your original patch $gmane/149542 here.

Thanks.