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.

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

[ValueTracking] Missed optimization of modulo power-of-two #92074

Open
YanWQ-monad opened this issue May 14, 2024 · 0 comments
Open

[ValueTracking] Missed optimization of modulo power-of-two #92074

YanWQ-monad opened this issue May 14, 2024 · 0 comments

Comments

@YanWQ-monad
Copy link
Contributor

Recently I found a missed optimization related to modulo power-of-two. It's

https://github.com/dtcxzyw/llvm-opt-benchmark/blob/b79d63cb8adea5949cc449d4186e342d3dbdd871/bench/wasmtime-rs/optimized/1ham0fg2czw77e66.ll#L235

and it's compiled from something like

let modulo = if size.is_power_of_two() {  // i.e. ctpop(size) == 1
    size * 2
} else {
    size.next_power_of_two()
};

let result = xxx % modulo;

where urem xxx, modulo could be optimized to and xxx, (modulo - 1).

To solve it, I have created a PR #91171 that enables the recognization of .next_power_of_two(), and I also have a draft patch that could recognize power-of-two from dom conditions.

However, the key problem in this case is that LLVM currently only search 2 levels beyond phi nodes, so the resursion of isKnownToBeAPowerOfTwo stops at size * 2, and fails to know size is a power-of-two from the condition.

// Recursively check all incoming values. Limit recursion to 2 levels, so
// that search complexity is limited to number of operands^2.
unsigned NewDepth = std::max(Depth, MaxAnalysisRecursionDepth - 1);

Though changing - 1 to - 3 could make it work in this case, it's obviously not a proper way to solve the problem.

I am not sure if there is a way to handle this complex context across multiple basic blocks, or should we instead patch wasmtime to use let modulo = (size + 1).next_power_of_two()?

cc @nikic @dtcxzyw

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants