{"action":"created","issue":{"url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/4942","repository_url":"https://api.github.com/repos/Beep6581/RawTherapee","labels_url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/4942/labels{/name}","comments_url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/4942/comments","events_url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/4942/events","html_url":"https://github.com/Beep6581/RawTherapee/issues/4942","id":377380189,"node_id":"MDU6SXNzdWUzNzczODAxODk=","number":4942,"title":"SSE: Review usage of vminf, vmaxf functions","user":{"login":"heckflosse","id":13733487,"node_id":"MDQ6VXNlcjEzNzMzNDg3","avatar_url":"https://avatars3.githubusercontent.com/u/13733487?v=4","gravatar_id":"","url":"https://api.github.com/users/heckflosse","html_url":"https://github.com/heckflosse","followers_url":"https://api.github.com/users/heckflosse/followers","following_url":"https://api.github.com/users/heckflosse/following{/other_user}","gists_url":"https://api.github.com/users/heckflosse/gists{/gist_id}","starred_url":"https://api.github.com/users/heckflosse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/heckflosse/subscriptions","organizations_url":"https://api.github.com/users/heckflosse/orgs","repos_url":"https://api.github.com/users/heckflosse/repos","events_url":"https://api.github.com/users/heckflosse/events{/privacy}","received_events_url":"https://api.github.com/users/heckflosse/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"heckflosse","id":13733487,"node_id":"MDQ6VXNlcjEzNzMzNDg3","avatar_url":"https://avatars3.githubusercontent.com/u/13733487?v=4","gravatar_id":"","url":"https://api.github.com/users/heckflosse","html_url":"https://github.com/heckflosse","followers_url":"https://api.github.com/users/heckflosse/followers","following_url":"https://api.github.com/users/heckflosse/following{/other_user}","gists_url":"https://api.github.com/users/heckflosse/gists{/gist_id}","starred_url":"https://api.github.com/users/heckflosse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/heckflosse/subscriptions","organizations_url":"https://api.github.com/users/heckflosse/orgs","repos_url":"https://api.github.com/users/heckflosse/repos","events_url":"https://api.github.com/users/heckflosse/events{/privacy}","received_events_url":"https://api.github.com/users/heckflosse/received_events","type":"User","site_admin":false},"assignees":[{"login":"heckflosse","id":13733487,"node_id":"MDQ6VXNlcjEzNzMzNDg3","avatar_url":"https://avatars3.githubusercontent.com/u/13733487?v=4","gravatar_id":"","url":"https://api.github.com/users/heckflosse","html_url":"https://github.com/heckflosse","followers_url":"https://api.github.com/users/heckflosse/followers","following_url":"https://api.github.com/users/heckflosse/following{/other_user}","gists_url":"https://api.github.com/users/heckflosse/gists{/gist_id}","starred_url":"https://api.github.com/users/heckflosse/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/heckflosse/subscriptions","organizations_url":"https://api.github.com/users/heckflosse/orgs","repos_url":"https://api.github.com/users/heckflosse/repos","events_url":"https://api.github.com/users/heckflosse/events{/privacy}","received_events_url":"https://api.github.com/users/heckflosse/received_events","type":"User","site_admin":false}],"milestone":null,"comments":10,"created_at":"2018-11-05T12:31:49Z","updated_at":"2018-11-05T18:01:09Z","closed_at":null,"author_association":"COLLABORATOR","body":"The SSE intrinsics _mm_min_ps(a, b) and _mm_max_ps(a, b) which are used in vminf(a, b) and vmaxf(a, b) return the second argument (b) if one of the arguments is a NaN.\r\n\r\nWe can use this fact to make for example the vectorized LUT access methods a bit more robust without impact on performance:\r\n\r\n```diff\r\ndiff --git a/rtengine/LUT.h b/rtengine/LUT.h\r\nindex d2f758689..9641f0e9e 100644\r\n--- a/rtengine/LUT.h\r\n+++ b/rtengine/LUT.h\r\n@@ -320,7 +320,7 @@ public:\r\n \r\n         // Clamp and convert to integer values. Extract out of SSE register because all\r\n         // lookup operations use regular addresses.\r\n-        vfloat clampedIndexes = vmaxf(ZEROV, vminf(maxsv, indexv));\r\n+        vfloat clampedIndexes = vmaxf(vminf(indexv, maxsv), ZEROV); // this automagically uses maxsv in case indexv is a nan\r\n         vint indexes = _mm_cvttps_epi32(clampedIndexes);\r\n         int indexArray[4];\r\n         _mm_storeu_si128(reinterpret_cast<__m128i*>(&indexArray[0]), indexes);\r\n@@ -352,7 +352,7 @@ public:\r\n \r\n         // Clamp and convert to integer values. Extract out of SSE register because all\r\n         // lookup operations use regular addresses.\r\n-        vfloat clampedIndexes = vmaxf(ZEROV, vminf(maxsv, indexv));\r\n+        vfloat clampedIndexes = vmaxf(vminf(indexv, maxsv), ZEROV); // this automagically uses maxsv in case indexv is a nan\r\n         vint indexes = _mm_cvttps_epi32(clampedIndexes);\r\n         int indexArray[4];\r\n         _mm_storeu_si128(reinterpret_cast<__m128i*>(&indexArray[0]), indexes);\r\n@@ -372,7 +372,7 @@ public:\r\n         vfloat lower = _mm_castsi128_ps(_mm_unpacklo_epi64(temp0, temp1));\r\n         vfloat upper = _mm_castsi128_ps(_mm_unpackhi_epi64(temp0, temp1));\r\n \r\n-        vfloat diff = vmaxf(ZEROV, vminf(sizev, indexv)) - _mm_cvtepi32_ps(indexes);\r\n+        vfloat diff = vmaxf(vminf(indexv, sizev), ZEROV) - _mm_cvtepi32_ps(indexes); // this automagically uses sizev in case indexv is a nan\r\n         return vintpf(diff, upper, lower);\r\n     }\r\n \r\n@@ -383,7 +383,7 @@ public:\r\n \r\n         // Clamp and convert to integer values. Extract out of SSE register because all\r\n         // lookup operations use regular addresses.\r\n-        vfloat clampedIndexes = vmaxf(ZEROV, vminf(maxsv, indexv));\r\n+        vfloat clampedIndexes = vmaxf(vminf(indexv, maxsv), ZEROV); // this automagically uses maxsv in case indexv is a nan\r\n         vint indexes = _mm_cvttps_epi32(clampedIndexes);\r\n         int indexArray[4];\r\n         _mm_storeu_si128(reinterpret_cast<__m128i*>(&indexArray[0]), indexes);\r\n@@ -420,7 +420,8 @@ public:\r\n     template<typename U = T, typename = typename std::enable_if<std::is_same<U, float>::value>::type>\r\n     vfloat operator[](vint idxv) const\r\n     {\r\n-        vfloat tempv = vmaxf(ZEROV, vminf(sizev, _mm_cvtepi32_ps(idxv))); // convert to float because SSE2 has no min/max for 32bit integers\r\n+        // convert to float because SSE2 has no min/max for 32bit integers\r\n+        vfloat tempv = vmaxf(vminf(_mm_cvtepi32_ps(idxv), sizev), ZEROV); // this automagically uses sizev in case indexv is a nan\r\n         idxv = _mm_cvttps_epi32(tempv);\r\n         // access the LUT 4 times. Trust the compiler. It generates good code here, better than hand written SSE code\r\n         return _mm_setr_ps(data[_mm_cvtsi128_si32(idxv)],\r\n\r\n\r\n```\r\n\r\n@Floessie Thoughts?"},"comment":{"url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/comments/435972797","html_url":"https://github.com/Beep6581/RawTherapee/issues/4942#issuecomment-435972797","issue_url":"https://api.github.com/repos/Beep6581/RawTherapee/issues/4942","id":435972797,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNTk3Mjc5Nw==","user":{"login":"Floessie","id":10133457,"node_id":"MDQ6VXNlcjEwMTMzNDU3","avatar_url":"https://avatars3.githubusercontent.com/u/10133457?v=4","gravatar_id":"","url":"https://api.github.com/users/Floessie","html_url":"https://github.com/Floessie","followers_url":"https://api.github.com/users/Floessie/followers","following_url":"https://api.github.com/users/Floessie/following{/other_user}","gists_url":"https://api.github.com/users/Floessie/gists{/gist_id}","starred_url":"https://api.github.com/users/Floessie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Floessie/subscriptions","organizations_url":"https://api.github.com/users/Floessie/orgs","repos_url":"https://api.github.com/users/Floessie/repos","events_url":"https://api.github.com/users/Floessie/events{/privacy}","received_events_url":"https://api.github.com/users/Floessie/received_events","type":"User","site_admin":false},"created_at":"2018-11-05T18:01:09Z","updated_at":"2018-11-05T18:01:09Z","author_association":"COLLABORATOR","body":"@heckflosse \r\n> Shall I make it consistent?\r\n\r\nThat would be beneficial. But please remember that we implemented `rtengine::min()` like this for a reason (better code by the compiler, on par with `std::min()`), so `rtengine::min()` shouldn't be modified.\r\n\r\nAs always, excellent work, Ingo! :+1: "}}
{"actor":{"display_login":"Floessie"}}