coffee/libyuv
1
0
Fork 0
mirror of https://chromium.googlesource.com/libyuv/libyuv synced 2026-06-26 20:38:50 +08:00
Code Issues Packages Projects Releases Wiki Activity
libyuv/298bd480ba48b445d8c185b4a660be96256f4b4a
Gerrit User 1115898 623f6658d8 Update patch set 2 
Patch Set 2:

(1 comment)

Patch-set: 2
Attention: {"person_ident":"Gerrit User 1115898 \u003c1115898@3ce6091f-6c88-37e8-8c75-72f92ae8dfba\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_1115898\u003e replied on the change"}
2024-11-12 21:27:04 +00:00

231 lines
12 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "a7afc81a_e3baaae0",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 10,
"author": {
"id": 1001562
},
"writtenOn": "2024-10-31T17:41:19Z",
"side": 1,
"message": "We need a Clang expert to check this statement. It\u0027s not clear to me whether \"-march\" and \"-Xclang -target-feature\" do not interfere with each other.",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "34e2fb10_03d68bba",
"filename": "/COMMIT_MSG",
"patchSetId": 2
},
"lineNbr": 10,
"author": {
"id": 1002219
},
"writtenOn": "2024-10-31T20:12:43Z",
"side": 1,
"message": "\u003e We need a Clang expert to check this statement. It\u0027s not clear to me whether \"-march\" and \"-Xclang -target-feature\" do not interfere with each other.\n\nI used this option to solve an Android build issue for libaom and libvpx where the `-march` flag was being overridden by the platform:\n\nhttps://android-review.googlesource.com/c/platform/external/libaom/+/3263691",
"parentUuid": "a7afc81a_e3baaae0",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "611101b9_908097ba",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 1177474
},
"writtenOn": "2024-10-23T08:25:08Z",
"side": 1,
"message": "Hi Wan-Teh, could you PTAL at this Chromium-specific BUILD.gn change for libyuv?\n\nThanks\nRichard",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "7bc67df5_ce6768c2",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 1001562
},
"writtenOn": "2024-10-31T17:34:11Z",
"side": 1,
"message": "Sorry about the delay in review. I was sick last week and somehow missed this CL.\n\nI am not really qualified to review this change, because I don\u0027t fully understand the problem we need to solve. Here are two initial thoughts.\n\n1) I am not sure if libyuv\u0027s BUILD.gn file can assume the compiler is Clang.\n\n2) I am wondering if it is the [proposed change to build/config/compiler/BUILD.gn](https://chromium-review.googlesource.com/c/chromium/src/+/5904093/1/build/config/compiler/BUILD.gn) that should be changed to use the Clang cc1 flag \"-target-feature\":\n\n\n```\nconfig(\"compiler_arm64_lse\") {\n if (current_cpu \u003d\u003d \"arm64\" \u0026\u0026 aarch64_use_lse) {\n cflags \u003d [ \"-march\u003darmv8-a+lse\" ]\n ldflags \u003d [ \"-march\u003darmv8-a+lse\" ]\n }\n}\n```",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "5a7b8249_2245e48a",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 1115898
},
"writtenOn": "2024-10-31T17:36:06Z",
"side": 1,
"message": "Does this still work for gcc and other compilers that support -march?",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "3d60ddd6_69381c4f",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 3668288
},
"writtenOn": "2024-11-04T11:40:23Z",
"side": 1,
"message": "No worries, I hope you feel better!\n\nTo explain the problem at hand: \nHaving a global config that uses the \"-march\" flag overwrites the flag args set by other build files resulting in an issue.\n\nRegarding:\n1) There is a variable \"is_clang\" that is being utilized in chromium build files which we can use (since \"target-feature\" seems to be not available in other compilers).\n\n2) Initially we tried that, but it seems that using \"-march\" flag to enable LSE enforces more implicit changes to the binaries which we are hoping would bring more uplift to performance (the end-goal of this patch). Since the features in libyuv such as i8mm or dotprod are being used explicitly however using \"-march\" or \"-target-feature\" doesn\u0027t make a difference.",
"parentUuid": "7bc67df5_ce6768c2",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "c6740c0b_8dc0f681",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 3668288
},
"writtenOn": "2024-11-04T11:40:23Z",
"side": 1,
"message": "Particularly in patchset 2 I believe it will be an issue for other compilers, but moving forward we can utilize this flag \"-target-feature\" only for clang and \"-march\" for the rest.",
"parentUuid": "5a7b8249_2245e48a",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "92f4896c_95c9dac7",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 1177474
},
"writtenOn": "2024-11-04T12:35:51Z",
"side": 1,
"message": "Worth mentioning: \na) Clang is the only supported compiler for doing Chromium stuff (I\u0027m not aware of any other projects using GN and GCC). GCC is best-effort supported.\nb) If GCC doesn\u0027t support this flag, I think we (Arm) should add it, but GCC moves a lot more slowly than LLVM and old GCC versions stick around for a long time. I wonder if is_clang could work for us in this situation.",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": false,
"key": {
"uuid": "75094aae_140fca03",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 2
},
"lineNbr": 0,
"author": {
"id": 1001629
},
"writtenOn": "2024-11-05T19:48:23Z",
"side": 1,
"message": "We should try not to use Xclang flags, even if we only support clang. They\u0027re not clang\u0027s \"stable API\", if you will.\n\nIs this really the only place where https://chromium-review.googlesource.com/c/chromium/src/+/5904093 collides with other target flags? I would\u0027ve expected that to be pretty common, since what libyuv does here seems like the \"normal\" approach for multiversioning a whole target at once.\n\nAnother idea could be to have a \"arm_base_arch\" variable in //build that we set to either \"armv8-a\" or \"armv8-a+lse\" and then pass `\"-march\u003d${arm_base_arch}+sme\"` (etc) here, and to pass \"-march\u003d${arm_base_arch}` globally in some config that we remove here? That should work with gcc too.",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "5d490a85_1b62ac9a",
"filename": "BUILD.gn",
"patchSetId": 2
},
"lineNbr": 259,
"author": {
"id": 1001629
},
"writtenOn": "2024-11-06T01:59:35Z",
"side": 1,
"message": "wtc pointed out that several of these targets have very few files and functions.\n\nAnother option then is to use `__attribute((target_version(\"sve\")))` in the source files on the functions, and not have a separate target with separate flags for these at all.\n\nThis used to not work great on arm, but with https://github.com/llvm/llvm-project/issues/56480 fixed this should be better.\n\nThis would have the advantage that we don\u0027t have to worry about ODR violations from inline functions in headers, too.\n\n(It\u0027d only help if libyuv is one of fairly few targets that do these per-target-feature optimizations. I\u0027d be a bit surprised if that\u0027s true. But if the attributes work, using them would be a nice change regardless of your specific project.)",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "f40e6fc1_47a15fa9",
"filename": "BUILD.gn",
"patchSetId": 2
},
"lineNbr": 259,
"author": {
"id": 1001562
},
"writtenOn": "2024-11-06T17:44:49Z",
"side": 1,
"message": "\u003e Another option then is to use `__attribute((target_version(\"sve\")))` in the source files on the functions, and not have a separate target with separate flags for these at all.\n\nDid you mean `__attribute` or `__attribute__`?\n\nThe `target_version` attribute doesn\u0027t seem to be supported by GCC:\nhttps://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html\nhttps://gcc.gnu.org/onlinedocs/gcc/AArch64-Function-Attributes.html\n\n\u003e (It\u0027d only help if libyuv is one of fairly few targets that do these per-target-feature optimizations. I\u0027d be a bit surprised if that\u0027s true. But if the attributes work, using them would be a nice change regardless of your specific project.)\n\nBoth libvpx and libaom also do these per-target-feature optimizations.\n\nhttps://source.chromium.org/chromium/chromium/src/+/main:third_party/libvpx/BUILD.gn;l\u003d354\n\nhttps://source.chromium.org/chromium/chromium/src/+/main:third_party/libaom/BUILD.gn;l\u003d249",
"parentUuid": "5d490a85_1b62ac9a",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "89bd57ac_2a7a3a43",
"filename": "BUILD.gn",
"patchSetId": 2
},
"lineNbr": 259,
"author": {
"id": 1001629
},
"writtenOn": "2024-11-06T19:32:51Z",
"side": 1,
"message": "`__attribute__`. `__attribute__(target())` seems to do the trick with both compilers:\n\n```\n$ cat test.cc\n#include \u003carm_acle.h\u003e\n\n__attribute__((target(\"+crc+nocrypto\")))\nuint32_t foo (uint32_t a, uint8_t b) {\n return __crc32b(a, b);\n}\n\nint main() {\n return foo(54, 8);\n}\n\n$ aarch64-linux-gnu-g++ test.cc -march\u003darmv8-a -S -o - | grep crc\n\t.arch armv8-a+crc\n\tcrc32b\tw0, w0, w1\n\n$ third_party/llvm-build/Release+Asserts/bin/clang++ --target\u003daarch64-linux-gnu test.cc -march\u003darmv8-a -S -o - | grep crc\n\tcrc32b\tw0, w8, w9\n```",
"parentUuid": "f40e6fc1_47a15fa9",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
},
{
"unresolved": true,
"key": {
"uuid": "504633b8_75c43b6c",
"filename": "BUILD.gn",
"patchSetId": 2
},
"lineNbr": 259,
"author": {
"id": 1115898
},
"writtenOn": "2024-11-12T21:27:04Z",
"side": 1,
"message": "attribute are overridden by command line and march and become compile errors.\nAlso they only work with gcc/clang syntax compilers",
"parentUuid": "89bd57ac_2a7a3a43",
"revId": "298bd480ba48b445d8c185b4a660be96256f4b4a",
"serverId": "3ce6091f-6c88-37e8-8c75-72f92ae8dfba"
}
]
}
Reference in New Issue View Git Blame Copy Permalink