* [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros
@ 2010-06-23 19:47 Ramsay Jones
2010-06-24 11:21 ` Jonathan Nieder
0 siblings, 1 reply; 5+ messages in thread
From: Ramsay Jones @ 2010-06-23 19:47 UTC (permalink / raw
To: Junio C Hamano; +Cc: GIT Mailing-list
On Intel machines, the msvc compiler defines the CPU architecture
macros _M_IX86 and _M_X64 (equivalent to __i386__ and __x86_64__
respectively). Use these macros in the pre-processor expression
to select the "fast" definition of the {get,put}_be32() macros.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
I did consider adding an additional guard, like so:
(defined(_MSC_VER) && (defined(_M_IX86) || defined(_M_X64))) || \
However, I decided that the extra paranoia was not needed ...
block-sha1/sha1.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
index d893475..e102856 100644
--- a/block-sha1/sha1.c
+++ b/block-sha1/sha1.c
@@ -70,6 +70,7 @@
*/
#if defined(__i386__) || defined(__x86_64__) || \
+ defined(_M_IX86) || defined(_M_X64) || \
defined(__ppc__) || defined(__ppc64__) || \
defined(__powerpc__) || defined(__powerpc64__) || \
defined(__s390__) || defined(__s390x__)
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros
2010-06-23 19:47 [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros Ramsay Jones
@ 2010-06-24 11:21 ` Jonathan Nieder
2010-06-25 20:13 ` Ramsay Jones
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2010-06-24 11:21 UTC (permalink / raw
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
Ramsay Jones wrote:
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index d893475..e102856 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -70,6 +70,7 @@
> */
>
> #if defined(__i386__) || defined(__x86_64__) || \
> + defined(_M_IX86) || defined(_M_X64) || \
> defined(__ppc__) || defined(__ppc64__) || \
> defined(__powerpc__) || defined(__powerpc64__) || \
> defined(__s390__) || defined(__s390x__)
Looks good to me, for what it’s worth. You might want a similar
change on line 57:
-#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))
Or alternatively, it might make sense to add something like the
following to compat/mingw.h or git-compat-util.h to fix this once.
#if defined(_M_IX86) && !defined(__i386__)
# define __i386__
#endif
#if defined(_M_X64) && !defined (__x86_64__)
# define __x86_64__
#endif
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros
2010-06-24 11:21 ` Jonathan Nieder
@ 2010-06-25 20:13 ` Ramsay Jones
2010-06-26 18:31 ` Jonathan Nieder
0 siblings, 1 reply; 5+ messages in thread
From: Ramsay Jones @ 2010-06-25 20:13 UTC (permalink / raw
To: Jonathan Nieder; +Cc: Junio C Hamano, GIT Mailing-list
Jonathan Nieder wrote:
> Ramsay Jones wrote:
>
>> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
>> index d893475..e102856 100644
>> --- a/block-sha1/sha1.c
>> +++ b/block-sha1/sha1.c
>> @@ -70,6 +70,7 @@
>> */
>>
>> #if defined(__i386__) || defined(__x86_64__) || \
>> + defined(_M_IX86) || defined(_M_X64) || \
>> defined(__ppc__) || defined(__ppc64__) || \
>> defined(__powerpc__) || defined(__powerpc64__) || \
>> defined(__s390__) || defined(__s390x__)
>
> Looks good to me, for what it’s worth. You might want a similar
> change on line 57:
>
>
> -#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).
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. In other words, maybe line 57 should read:
#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__))
I don't know ... anyway I suspect that msvc has a different set of
code generation quirks! :-P
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 ;-).
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros
2010-06-25 20:13 ` Ramsay Jones
@ 2010-06-26 18:31 ` Jonathan Nieder
2010-06-30 19:53 ` Ramsay Jones
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Nieder @ 2010-06-26 18:31 UTC (permalink / raw
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
Ramsay Jones wrote:
> Jonathan Nieder wrote:
>> -#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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros
2010-06-26 18:31 ` Jonathan Nieder
@ 2010-06-30 19:53 ` Ramsay Jones
0 siblings, 0 replies; 5+ messages in thread
From: Ramsay Jones @ 2010-06-30 19:53 UTC (permalink / raw
To: Jonathan Nieder; +Cc: Junio C Hamano, GIT Mailing-list
Jonathan Nieder wrote:
>> 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>
Thanks!
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-06-30 20:33 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-23 19:47 [PATCH 2/4] msvc: Select the "fast" definition of the {get,put}_be32() macros Ramsay Jones
2010-06-24 11:21 ` Jonathan Nieder
2010-06-25 20:13 ` Ramsay Jones
2010-06-26 18:31 ` Jonathan Nieder
2010-06-30 19:53 ` Ramsay Jones
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).