Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compile errors on clang and Power #129

Merged
merged 7 commits into from
Sep 16, 2022
Merged

Fix compile errors on clang and Power #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ef66877
[VSX] clang complains about the order of __vector
markos Sep 16, 2022
3fc6c8a
[VSX] movemask needs to be explicitly aligned on clang for vec_ste
markos Sep 16, 2022
6de45b4
clang 14 complains about this, needs investigation
markos Sep 16, 2022
0e0147e
clang 14 does not allow bitwise OR for bools
markos Sep 16, 2022
a4972aa
remove leftover debug print
markos Sep 16, 2022
911a98d
clang 13+ gives wrong -Wunused-but-set-variable error on nfa/mcclella…
markos Sep 16, 2022
48105cd
move variable
markos Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
cmake_minimum_required (VERSION 2.8.11)
cmake_minimum_required (VERSION 2.8.12)

project (vectorscan C CXX)

set (HS_MAJOR_VERSION 5)
Expand Down Expand Up @@ -296,6 +297,12 @@ if (NOT RELEASE_BUILD)
# release builds
set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Werror")
set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Werror")
if (CMAKE_COMPILER_IS_CLANG)
if (CMAKE_C_COMPILER_VERSION VERSION_GREATER "13.0")
set(EXTRA_C_FLAGS "${EXTRA_C_FLAGS} -Wno-unused-but-set-variable")
set(EXTRA_CXX_FLAGS "${EXTRA_CXX_FLAGS} -Wno-unused-but-set-variable")
endif()
endif()
endif()

if (DISABLE_ASSERTS)
Expand Down
2 changes: 1 addition & 1 deletion src/nfa/mcclellancompile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1484,12 +1484,12 @@ bytecode_ptr<NFA> mcclellanCompile_i(raw_dfa &raw, accel_dfa_build_strat &strat,
find_wide_state(info);
}

u16 total_daddy = 0;
bool any_cyclic_near_anchored_state
= is_cyclic_near(raw, raw.start_anchored);

// Sherman optimization
if (info.impl_alpha_size > 16) {
u16 total_daddy = 0;
for (u32 i = 0; i < info.size(); i++) {
if (info.is_widestate(i)) {
continue;
Expand Down
3 changes: 1 addition & 2 deletions src/nfagraph/ng_misc_opt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,8 +385,7 @@ bool improveGraph(NGHolder &g, som_type som) {

const vector<NFAVertex> ordering = getTopoOrdering(g);

return enlargeCyclicCR(g, som, ordering)
| enlargeCyclicCR_rev(g, ordering);
return enlargeCyclicCR(g, som, ordering) || enlargeCyclicCR_rev(g, ordering);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not equivalent, FYI. Graph g can be changed in both directions with | rather than ||

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang 14 complains if you use bitwise OR on booleans, hence the change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless you mean for || the compiler only has to evaluate the first to be true for optimization, so the second may not be executed. In which case, I will probably have to evaluate both separately and just OR the results.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's what @danlark1 meant: both sides may need to be evaluated for their side effects, so switching from | to || could be incorrect.

It would be safer to rewrite this instead like you mentioned, ORing the results:

Suggested change
return enlargeCyclicCR(g, som, ordering) || enlargeCyclicCR_rev(g, ordering);
bool lhs = enlargeCyclicCR(g, som, ordering);
bool rhs = enlargeCyclicCR_rev(g, ordering);
return lhs || rhs;

}

/** finds a smaller reachability for a state by the reverse transformation of
Expand Down
4 changes: 2 additions & 2 deletions src/rose/rose_build_add.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,9 @@ RoseRoleHistory selectHistory(const RoseBuildImpl &tbi, const RoseBuildData &bd,
const bool fixed_offset_src = g[u].fixedOffset();
const bool has_bounds = g[e].minBound || (g[e].maxBound != ROSE_BOUND_INF);

DEBUG_PRINTF("edge %zu->%zu, bounds=[%u,%u], fixed_u=%d, prefix=%d\n",
/*DEBUG_PRINTF("edge %zu->%zu, bounds=[%u,%u], fixed_u=%d, prefix=%d\n",
g[u].index, g[v].index, g[e].minBound, g[e].maxBound,
(int)g[u].fixedOffset(), (int)g[v].left);
(int)g[u].fixedOffset(), (int)g[v].left);*/

if (g[v].left) {
// Roles with prefix engines have their history handled by that prefix.
Expand Down
34 changes: 17 additions & 17 deletions src/util/arch/ppc64el/simd_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ static really_inline u32 movemask128(m128 a) {
static uint8x16_t perm = { 16, 24, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
uint8x16_t bitmask = vec_gb((uint8x16_t) a);
bitmask = (uint8x16_t) vec_perm(vec_splat_u8(0), bitmask, perm);
u32 movemask;
u32 ALIGN_ATTR(16) movemask;
vec_ste((uint32x4_t) bitmask, 0, &movemask);
return movemask;
}
Expand Down Expand Up @@ -285,27 +285,27 @@ m128 loadbytes128(const void *ptr, unsigned int n) {
return a;
}

#define CASE_ALIGN_VECTORS(a, b, offset) case offset: return (m128)vec_sld((int8x16_t)(b), (int8x16_t)(a), (16 - offset)); break;
#define CASE_ALIGN_VECTORS(a, b, offset) case offset: return (m128)vec_sld((int8x16_t)(a), (int8x16_t)(b), (16 - offset)); break;

static really_really_inline
m128 palignr_imm(m128 r, m128 l, int offset) {
switch (offset) {
case 0: return l; break;
CASE_ALIGN_VECTORS(l, r, 1);
CASE_ALIGN_VECTORS(l, r, 2);
CASE_ALIGN_VECTORS(l, r, 3);
CASE_ALIGN_VECTORS(l, r, 4);
CASE_ALIGN_VECTORS(l, r, 5);
CASE_ALIGN_VECTORS(l, r, 6);
CASE_ALIGN_VECTORS(l, r, 7);
CASE_ALIGN_VECTORS(l, r, 8);
CASE_ALIGN_VECTORS(l, r, 9);
CASE_ALIGN_VECTORS(l, r, 10);
CASE_ALIGN_VECTORS(l, r, 11);
CASE_ALIGN_VECTORS(l, r, 12);
CASE_ALIGN_VECTORS(l, r, 13);
CASE_ALIGN_VECTORS(l, r, 14);
CASE_ALIGN_VECTORS(l, r, 15);
CASE_ALIGN_VECTORS(r, l, 1);
CASE_ALIGN_VECTORS(r, l, 2);
CASE_ALIGN_VECTORS(r, l, 3);
CASE_ALIGN_VECTORS(r, l, 4);
CASE_ALIGN_VECTORS(r, l, 5);
CASE_ALIGN_VECTORS(r, l, 6);
CASE_ALIGN_VECTORS(r, l, 7);
CASE_ALIGN_VECTORS(r, l, 8);
CASE_ALIGN_VECTORS(r, l, 9);
CASE_ALIGN_VECTORS(r, l, 10);
CASE_ALIGN_VECTORS(r, l, 11);
CASE_ALIGN_VECTORS(r, l, 12);
CASE_ALIGN_VECTORS(r, l, 13);
CASE_ALIGN_VECTORS(r, l, 14);
CASE_ALIGN_VECTORS(r, l, 15);
case 16: return r; break;
default: return zeroes128(); break;
}
Expand Down
6 changes: 3 additions & 3 deletions src/util/supervector/arch/ppc64el/impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ really_inline SuperVector<16>::SuperVector(SuperVector const &other)

template<>
template<>
really_inline SuperVector<16>::SuperVector(char __bool __vector v)
really_inline SuperVector<16>::SuperVector(__vector __bool char v)
{
u.u8x16[0] = (uint8x16_t) v;
};
Expand Down Expand Up @@ -269,10 +269,10 @@ really_inline SuperVector<16> SuperVector<16>::eq(SuperVector<16> const &b) cons
template <>
really_inline typename SuperVector<16>::comparemask_type
SuperVector<16>::comparemask(void) const {
uint8x16_t bitmask = vec_gb( u.u8x16[0]);
static uint8x16_t perm = { 16, 24, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };
uint8x16_t bitmask = vec_gb(u.u8x16[0]);
bitmask = (uint8x16_t) vec_perm(vec_splat_u8(0), bitmask, perm);
u32 movemask;
u32 ALIGN_ATTR(16) movemask;
vec_ste((uint32x4_t) bitmask, 0, &movemask);
return movemask;
}
Expand Down
2 changes: 0 additions & 2 deletions src/util/supervector/arch/x86/impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -523,9 +523,7 @@ template <>
really_inline SuperVector<16> SuperVector<16>::loadu_maskz(void const *ptr, uint8_t const len)
{
SuperVector mask = Ones_vshr(16 -len);
mask.print8("mask");
SuperVector v = _mm_loadu_si128((const m128 *)ptr);
v.print8("v");
return mask & v;
}

Expand Down