Skip to content

Missed optimization: llvm unable to remove bounds check #48253

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

Closed
alex mannequin opened this issue Jan 27, 2021 · 4 comments
Closed

Missed optimization: llvm unable to remove bounds check #48253

alex mannequin opened this issue Jan 27, 2021 · 4 comments
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations

Comments

@alex
Copy link
Mannequin

alex mannequin commented Jan 27, 2021

Bugzilla Link 48909
Version unspecified
OS All
CC @davidbolvansky,@fhahn,@LebedevRI,@nelhage,@nikic

Extended Description

https://godbolt.org/z/vEfPYj (also below)

LLVM should be able to remove the branch that leads to the call to abort() -- data.length is known to be the same as block_len, and block_len is known not to be 0. Therefore llvm should know that block_len - is always < data.length.

Source:

#include <stdlib.h>
#include <stdbool.h>
#include <stdint.h>

typedef struct {
    uint8_t *data;
    size_t length;
} slice;

static inline uint8_t subscript(slice data, size_t index) {
    if (index >= data.length) {
        abort();
    }
    return data.data[index];
}

bool check_padding(slice data, size_t block_len) {
    if (data.length != block_len || block_len == 0) {
        return false;
    }

    uint8_t pad_size = subscript(data, block_len - 1);
    return pad_size > 7;
}

generated assembly:

check_padding:                          # @check_padding
        push    rax
        xor     eax, eax
        cmp     rsi, rdx
        jne     .LBB0_4
        test    rdx, rdx
        je      .LBB0_4
        test    rsi, rsi
        je      .LBB0_5
        cmp     byte ptr [rsi + rdi - 1], 7
        seta    al
.LBB0_4:
        pop     rcx
        ret
.LBB0_5:
        call    abort
@nelhage
Copy link

nelhage commented Jan 27, 2021

I compiled to LLVM IR and cleaned up the result a bit and put it through alive2; Assuming I did this right this verifies that there aren't any subtle UB or poison edge cases here, the proposed straightforward optimization is correct at the LLVM layer.

https://alive2.llvm.org/ce/z/Ckgak9

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
fhahn added a commit that referenced this issue Feb 11, 2022
fhahn added a commit that referenced this issue Feb 11, 2022
If we can prove that an addition without wrap flags won't wrap, decompse
the operation.

Issue #48253
@fhahn
Copy link
Contributor

fhahn commented Feb 12, 2022

After 66400fc, this should get optimized as expected with -mllvm -enable-constraint-elimination: https://godbolt.org/z/ev51WehaM

@alex
Copy link
Member

alex commented Feb 12, 2022

Great, thank you! I guess this will be closed when #49344 is completed?

@fhahn
Copy link
Contributor

fhahn commented Feb 14, 2022

Great, thank you! I guess this will be closed when #49344 is completed?

Yes. I' planning to propose the pass to be enabled by default soon.

@fhahn fhahn closed this as completed in fb13dcf Jan 4, 2023
fhahn added a commit that referenced this issue Feb 6, 2023
This reverts commit 695ce48.

The compile-time regression causing the revert has been fixed. Recommit
the original patch.

Original commit message:

   The pass should help to close a functional gap when it comes to
    reasoning about related conditions in a relatively general way.

    It addresses multiple existing issues (linked below) and the need for a
    more powerful reasoning system was also discussed recently in
    https://discourse.llvm.org/t/rfc-alternative-approach-of-dealing-with-implications-from-comparisons-through-pos-analysis/65601/7

    On AArch64, the new pass performs ~2000 simplifications on
    MultiSource,SPEC2006,SPEC2017 with -O3.

    Compile-time impact:

    NewPM-O3: +0.20%
    NewPM-ReleaseThinLTO: +0.32%
    NewPM-ReleaseLTO-g: +0.28%

    https://llvm-compile-time-tracker.com/compare.php?from=f01a3a893c147c1594b9a3fbd817456b209dabbf&to=577688758ef64fb044215ec3e497ea901bb2db28&stat=instructions:u

    Fixes #49344.
    Fixes #47888.
    Fixes #48253.
    Fixes #49229.
    Fixes #58074.

    Reviewed By: asbirlea

    Differential Revision: https://reviews.llvm.org/D135915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla llvm:optimizations
Projects
None yet
Development

No branches or pull requests

4 participants