Add vendor identity check for Hygon Dhyana processor in Scudo

[fanjinke]

Summary

The Hygon Dhyana processor supports hardware CRC32.

Related link:
https://reviews.llvm.org/D78874

Result of "make check":
Testing Time: 1364.04s

Unsupported Tests:   317
Expected Passes  : 36802
Expected Failures:   161

[100%] Built target check-llvm
[100%] Built target check

[cryptoad]

Regarding the Scudo side of the patch: the code has to be able to compile with gcc as well, and not necessarily the latest version.
This won't compile on systems without a signature_HYGON_*.

[fanjinke]

Hi,

Please help with the patch reveiw.

Hi cryptoad,
@cryptoad IS there anything incorrectly?

[fanjinke]

Results of "make check":

Testing Time: 1153.40s
  Expected Passes    : 36042
  Expected Failures  : 163
  Unsupported Tests  : 340
[100%] Built target check-llvm
Scanning dependencies of target check
[100%] Built target check

The compiler-rt directory can be compiled using gcc.

ChangeLog:
v2:

• Updated the scudo part of the patch, Thanks Cryptoad!

[fanjinke]

Hi Cryptoad,

Thanks for your reminds, and I get the points now. Then updated the scudo part of the patch.
After patch update，compiler-rt can be successfully compiled using gcc.

Thanks again!

[fanjinke]

Hi cryptoad，

Any suggestions?

[fanjinke]

1，Update patch base on lastest commit e90fb82f0f760703c14eafbad96c08b6019a2f0f.
2，Format the patch with “git clang-format-6.0 HEAD~1“.

[cryptoad]

Hey,

clang/lib/Headers/cpuid.h would have to be in its own CL that would have to be sent separately from the Scudo one.
It would have to be reviewed by clang people and likely some tests added.

Once this is done and landed, then the Scudo part can happen.

Thanks!

[fanjinke]

Hi Cryptoad,

Thank you so much for your comment. I will divide it into two patches according to your suggestion.

Best regards.

[craig.topper]

compiler-rt/lib/scudo/scudo_utils.cpp

85

What's the rationale for the vendor check here anyway? Why isn't the bit in ecx sufficient?

[fanjinke]

compiler-rt/lib/scudo/scudo_utils.cpp

85

Using the cpuid instruction to get the vendor id will return the ASCII code of the vendor id, which is stored in the ebx,ecx,edx registers.
The ASCII code in the Hygon CPU is "HygonGenuine", the ecx = "eniu".
For better differentiation from other cpus in the future, by following AMD/Intel way, we use full ASCII code to identify Hygon CPU.

[craig.topper]

compiler-rt/lib/scudo/scudo_utils.cpp

85

Sorry, my question was about why this was restricted to Intel/AMD in the first place. Why should this code need to be updated every time a new vendor comes along? Why isn’t checking for sse4.2 regardless of vendor sufficient.

[fanjinke]

compiler-rt/lib/scudo/scudo_utils.cpp

85

For this question, original author[1] may be more appropriate to reply.
From CPU specification, Bit20 of CPUID Fn0000_0001_ecx is SSE42 on Intel, AMD, and Hygon CPUs, but we are not sure that this is true for all x86 vendors.

Cryptoad, Any comments?

[1]: https://reviews.llvm.org/D40322

[fanjinke]

Changelog:
v3:
Remove the changes in cpuid.h that have already been committed in [1].

[1]: https://reviews.llvm.org/D78874

[cryptoad]

A couple of style issues to address.

compiler-rt/lib/scudo/scudo_utils.cpp

66

s/the gcc/gcc/

67

Please use C++-style comments (https://llvm.org/docs/CodingStandards.html#comment-formatting)

compiler-rt/lib/scudo/standalone/checksum.cpp

34

s/the gcc/gcc

35

Please use C++-style comments (https://llvm.org/docs/CodingStandards.html#comment-formatting)

[fanjinke]

Changelog:
v4:
Change the comments to C++ style. Thanks Cryptoad.

[fanjinke]

Hi Cryptoad，

Could you help me to commit the patch? Because I don't have access.

Thanks.

[cryptoad]

In D62368#2027895, @fanjinke wrote:

Hi Cryptoad，

Could you help me to commit the patch? Because I don't have access.

Thanks.

Done, thank you!

[craig.topper]

I noticed when I pulled this morning that this seems to have been pushed as a branch to the repo

From https://github.com/llvm/llvm-project

eb7d32e..63a4fdd  master     -> origin/master

• [new branch] arcpatch-D62368 -> origin/arcpatch-D62368

[cryptoad]

In D62368#2029752, @craig.topper wrote:

I noticed when I pulled this morning that this seems to have been pushed as a branch to the repo

From https://github.com/llvm/llvm-project

eb7d32e..63a4fdd  master     -> origin/master

• [new branch] arcpatch-D62368 -> origin/arcpatch-D62368

Hey Craig, I gave a try to arc land this morning as opposed to my regular git llvm push workflow.
The initial attempt failed at some point so I went back to git llvm push which succeeded (https://github.com/llvm/llvm-project/commit/9959eb918acfd37ce89d4a7082ac9957d3628f16#diff-483266ff26e7d1b17130f1b371c4e62d).
Is there something I have to clean up somewhere for that other branch?

[craig.topper]

In D62368#2029870, @cryptoad wrote:

In D62368#2029752, @craig.topper wrote:

I noticed when I pulled this morning that this seems to have been pushed as a branch to the repo

From https://github.com/llvm/llvm-project

eb7d32e..63a4fdd  master     -> origin/master

• [new branch] arcpatch-D62368 -> origin/arcpatch-D62368

Hey Craig, I gave a try to arc land this morning as opposed to my regular git llvm push workflow.
The initial attempt failed at some point so I went back to git llvm push which succeeded (https://github.com/llvm/llvm-project/commit/9959eb918acfd37ce89d4a7082ac9957d3628f16#diff-483266ff26e7d1b17130f1b371c4e62d).
Is there something I have to clean up somewhere for that other branch?

I've deleted the branch in the github web interface. I didn't know I could do that.

[cryptoad]

In D62368#2030005, @craig.topper wrote:

I've deleted the branch in the github web interface. I didn't know I could do that.

Ok thank you!

compiler-rt/lib/scudo/scudo_utils.cpp

 }
 
 #if defined(__x86_64__) || defined(__i386__)
 // i386 and x86_64 specific code to detect CRC32 hardware support via CPUID.
 // CRC32 requires the SSE 4.2 instruction set.
 # ifndef bit_SSE4_2
 #  define bit_SSE4_2 bit_SSE42  // clang and gcc have different defines.
 # endif
+
+#ifndef signature_HYGON_ebx // They are not defined in gcc.
+// HYGON: "HygonGenuine".
+#define signature_HYGON_ebx 0x6f677948
+#define signature_HYGON_edx 0x6e65476e
+#define signature_HYGON_ecx 0x656e6975
+#endif
+
 bool hasHardwareCRC32() {
   u32 Eax, Ebx, Ecx, Edx;
   __get_cpuid(0, &Eax, &Ebx, &Ecx, &Edx);
   const bool IsIntel = (Ebx == signature_INTEL_ebx) &&
                        (Edx == signature_INTEL_edx) &&
                        (Ecx == signature_INTEL_ecx);
   const bool IsAMD = (Ebx == signature_AMD_ebx) &&
                      (Edx == signature_AMD_edx) &&
                      (Ecx == signature_AMD_ecx);
-  if (!IsIntel && !IsAMD)
+  const bool IsHygon = (Ebx == signature_HYGON_ebx) &&
+                       (Edx == signature_HYGON_edx) &&
+                       (Ecx == signature_HYGON_ecx);
+  if (!IsIntel && !IsAMD && !IsHygon)
     return false;
   __get_cpuid(1, &Eax, &Ebx, &Ecx, &Edx);
   return !!(Ecx & bit_SSE4_2);
 }
 #elif defined(__arm__) || defined(__aarch64__)
 // For ARM and AArch64, hardware CRC32 support is indicated in the AT_HWCAP
 // auxiliary vector.
 # ifndef AT_HWCAP

compiler-rt/lib/scudo/standalone/checksum.cpp

 
 #if defined(__x86_64__) || defined(__i386__)
 // i386 and x86_64 specific code to detect CRC32 hardware support via CPUID.
 // CRC32 requires the SSE 4.2 instruction set.
 #ifndef bit_SSE4_2
 #define bit_SSE4_2 bit_SSE42 // clang and gcc have different defines.
 #endif
 
+#ifndef signature_HYGON_ebx // They are not defined in gcc.
+// HYGON: "HygonGenuine".
+#define signature_HYGON_ebx 0x6f677948
+#define signature_HYGON_edx 0x6e65476e
+#define signature_HYGON_ecx 0x656e6975
+#endif
+
 bool hasHardwareCRC32() {
   u32 Eax, Ebx = 0, Ecx = 0, Edx = 0;
   __get_cpuid(0, &Eax, &Ebx, &Ecx, &Edx);
   const bool IsIntel = (Ebx == signature_INTEL_ebx) &&
                        (Edx == signature_INTEL_edx) &&
                        (Ecx == signature_INTEL_ecx);
   const bool IsAMD = (Ebx == signature_AMD_ebx) && (Edx == signature_AMD_edx) &&
                      (Ecx == signature_AMD_ecx);
-  if (!IsIntel && !IsAMD)
+  const bool IsHygon = (Ebx == signature_HYGON_ebx) &&
+                       (Edx == signature_HYGON_edx) &&
+                       (Ecx == signature_HYGON_ecx);
+  if (!IsIntel && !IsAMD && !IsHygon)
     return false;
   __get_cpuid(1, &Eax, &Ebx, &Ecx, &Edx);
   return !!(Ecx & bit_SSE4_2);
 }
 #elif defined(__arm__) || defined(__aarch64__)
 #ifndef AT_HWCAP
 #define AT_HWCAP 16
 #endif