You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/09/09 00:19:45 UTC
[GitHub] [arrow] felipecrv opened a new pull request, #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
felipecrv opened a new pull request, #37638:
URL: https://github.com/apache/arrow/pull/37638
### Rationale for this change
We need to re-enable codepaths that use AVX2 without also enabling BMI2 instructions.
### What changes are included in this PR?
This PR is very much work-in-progress. I'm sending individual commits to probe CI for build issues.
- [x] Move PEXT software implementation from parquet to `arrow/util`
- [ ] ...
### Are these changes tested?
No new tests needed yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324476554
##########
cpp/src/arrow/util/bit_util_extract.cc:
##########
@@ -0,0 +1,247 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cstdint>
+
+#include "arrow/util/bit_util_extract.h"
+
+namespace arrow::bit_util {
+
+// clang-format off
+/* Python code to generate lookup table:
Review Comment:
`kPextTable` as a raw C array is entirely contiguous. Is it also the case for a nested `std::array`?
Also, what is the compilation time of your suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324704733
##########
cpp/src/arrow/util/bit_util_extract.cc:
##########
@@ -0,0 +1,247 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cstdint>
+
+#include "arrow/util/bit_util_extract.h"
+
+namespace arrow::bit_util {
+
+// clang-format off
+/* Python code to generate lookup table:
Review Comment:
I think the only thing we might need to unit test is ExtractBitsSoftwareFallback. We don't care about the contents of the table so long as it produces correct output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324567566
##########
cpp/src/arrow/util/bit_util_extract.cc:
##########
@@ -0,0 +1,247 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cstdint>
+
+#include "arrow/util/bit_util_extract.h"
+
+namespace arrow::bit_util {
+
+// clang-format off
+/* Python code to generate lookup table:
Review Comment:
std::array is a *very* thin wrapper around raw C arrays, so thin that it will not initialize trivial elements unless instructed:
```c++
int c[10]; // no initializer, array elements not zeroed
std::array<int, 10> cxx; // also
```
Compilation is fast enough that I can't get a consistent number for this example by itself; I'd need to rewrite it to a larger loop or something. Typically 8-16ms on my machine, which is the same range I see for the large array literal. In addition we only compile this once since we're not in a header
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324477705
##########
cpp/src/arrow/util/bit_util_extract.h:
##########
@@ -0,0 +1,77 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+#include "arrow/util/bit_util.h"
+#include "arrow/util/simd.h"
+
+namespace arrow::bit_util {
Review Comment:
I think the `bit_util` namespace is ok. As for the filename, perhaps make it `bit_util_extract_internal.h`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1320439353
##########
cpp/src/arrow/util/bit_util_extract.h:
##########
@@ -0,0 +1,77 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+#include "arrow/util/bit_util.h"
+#include "arrow/util/simd.h"
+
+namespace arrow::bit_util {
Review Comment:
@pitrou I need a filename suggestion here. And is it OK to put this in the `bit_util` namespace?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] felipecrv commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324617840
##########
cpp/src/arrow/util/bit_util_extract.cc:
##########
@@ -0,0 +1,247 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cstdint>
+
+#include "arrow/util/bit_util_extract.h"
+
+namespace arrow::bit_util {
+
+// clang-format off
+/* Python code to generate lookup table:
Review Comment:
The Python code exists here as documentation of where the table comes from. It's not something we plan to keep changing. If I replace the hardcoded constant table with this C++ code I will have to add tests to ensure the code is correct and these tests will have to include the table. And worse, I will have to expose this `constexpr` in a way that is accessible to tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a diff in pull request #37638: GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #37638:
URL: https://github.com/apache/arrow/pull/37638#discussion_r1324469134
##########
cpp/src/arrow/util/bit_util_extract.cc:
##########
@@ -0,0 +1,247 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <cstdint>
+
+#include "arrow/util/bit_util_extract.h"
+
+namespace arrow::bit_util {
+
+// clang-format off
+/* Python code to generate lookup table:
Review Comment:
FWIW:
```c++
constexpr auto kPextTable = [] {
std::array<std::array<uint8_t, 1 << kLookupBits>, 1 << kLookupBits> out{};
for (uint8_t mask = 0; mask < 1 << kLookupBits; ++mask) {
for (uint8_t data = 0; data < 1 << kLookupBits; ++data) {
uint8_t bit_value = 0, bit_len = 0;
for (uint8_t i = 0; i < kLookupBits; ++i) {
if (mask & (1 << i)) {
bit_value |= ((data >> i) & 1) << bit_len;
++bit_len;
}
}
// uncomment to assert it's the same table
// if (kPextTable_old[mask][data] != bit_value) throw 0;
out[mask][data] = bit_value;
}
}
return out;
}();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-37623: [C++] Use software implementations of BMI2 instructions when they are unavailable [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #37638:
URL: https://github.com/apache/arrow/pull/37638#issuecomment-3547444490
Thank you for your contribution. Unfortunately, this
pull request has been marked as stale because it has had no activity in the past 365 days. Please remove the stale label
or comment below, or this PR will be closed in 14 days. Feel free to re-open this if it has been closed in error. If you
do not have repository permissions to reopen the PR, please tag a maintainer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org