This is an archive of the discontinued LLVM Phabricator instance.

[X86][BMI1] X86DAGToDAGISel: select BEXTR from x << (32 - y) >> (32 - y) pattern
ClosedPublic

Authored by lebedev.ri on Oct 22 2018, 12:42 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri added inline comments.Oct 22 2018, 12:45 PM
test/CodeGen/X86/extract-lowbits.ll
2248 ↗(On Diff #170477)

This does not appear to be a miscompile:

$ /build/llvm-build-GCC-release/bin/llc -mtriple=i686-unknown-linux-gnu -mattr=+bmi,+tbm,+bmi2 test.ll -o -
        .text
        .file   "test.ll"
        .globl  bzhi32_d2_load          # -- Begin function bzhi32_d2_load
        .p2align        4, 0x90
        .type   bzhi32_d2_load,@function
bzhi32_d2_load:                         # @bzhi32_d2_load
# %bb.0:
        movl    4(%esp), %eax
        movl    8(%esp), %ecx
        bzhil   %ecx, (%eax), %eax
        retl
.Lfunc_end0:
        .size   bzhi32_d2_load, .Lfunc_end0-bzhi32_d2_load
                                        # -- End function

        .section        ".note.GNU-stack","",@progbits
$ llc-8 -mtriple=i686-unknown-linux-gnu -mattr=+bmi,+tbm,+bmi2 test.ll -o -
        .text
        .file   "test.ll"
        .globl  bzhi32_d2_load          # -- Begin function bzhi32_d2_load
        .p2align        4, 0x90
        .type   bzhi32_d2_load,@function
bzhi32_d2_load:                         # @bzhi32_d2_load
# %bb.0:
        movl    8(%esp), %eax
        movl    4(%esp), %ecx
        bzhil   %eax, (%ecx), %eax
        retl
.Lfunc_end0:
        .size   bzhi32_d2_load, .Lfunc_end0-bzhi32_d2_load
                                        # -- End function

        .section        ".note.GNU-stack","",@progbits
$ cat test.ll 
define i32 @bzhi32_d2_load(i32* %w, i32 %numlowbits) nounwind {
  %val = load i32, i32* %w
  %numhighbits = sub i32 32, %numlowbits
  %highbitscleared = shl i32 %val, %numhighbits
  %masked = lshr i32 %highbitscleared, %numhighbits
  ret i32 %masked
}
RKSimon added inline comments.Oct 22 2018, 1:40 PM
lib/Target/X86/X86ISelDAGToDAG.cpp
2705 ↗(On Diff #170477)

Size = NVT.getSizeInBits();

lebedev.ri marked an inline comment as done.

Use getSizeInBits().

This revision is now accepted and ready to land.Oct 22 2018, 3:30 PM

LGTM

Thank you for the review.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Oct 23 2018, 4:23 AM
lebedev.ri added a subscriber: vitalybuka.

Reverted in rL345017.
It looks like there is some miscompile here, since
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/25249/steps/check-lld%20msan/logs/stdio

+ ninja check-lld
[1/3] Linking CXX executable tools/lld/unittests/DriverTests/DriverTests
[2/3] Linking CXX executable tools/lld/unittests/MachOTests/lldMachOTests
[2/3] Running lld test suite
-- Testing: 1914 tests, 64 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70..
FAIL: lld :: ELF/relocatable-versioned.s (1518 of 1914)
******************** TEST 'lld :: ELF/relocatable-versioned.s' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /b/sanitizer-x86_64-linux-fast/build/llvm/tools/lld/test/ELF/relocatable-versioned.s -o /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/lld/test/ELF/Output/relocatable-versioned.s.tmp1.o
: 'RUN: at line 3';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/ld.lld -o /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/lld/test/ELF/Output/relocatable-versioned.s.tmp2.o -r /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/lld/test/ELF/Output/relocatable-versioned.s.tmp1.o
: 'RUN: at line 4';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/llvm-nm /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/tools/lld/test/ELF/Output/relocatable-versioned.s.tmp2.o | /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/FileCheck /b/sanitizer-x86_64-linux-fast/build/llvm/tools/lld/test/ELF/relocatable-versioned.s
--
Exit Code: 77

Command Output (stderr):
--
==17758==MemorySanitizer CHECK failed: /b/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:191 "((kBlockMagic)) == ((((u64*)addr)[0]))" (0x6a6cb03abcebc041, 0x0)
    #0 0x59716b in MsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /b/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/msan/msan.cc:393
    #1 0x586635 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /b/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_termination.cc:79
    #2 0x57d5ff in __sanitizer::InternalFree(void*, __sanitizer::SizeClassAllocatorLocalCache<__sanitizer::SizeClassAllocator32<__sanitizer::AP32> >*) /b/sanitizer-x86_64-linux-fast/build/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:191
    #3 0x7fc21b24193f  (/lib/x86_64-linux-gnu/libc.so.6+0x3593f)
    #4 0x7fc21b241999 in exit (/lib/x86_64-linux-gnu/libc.so.6+0x35999)
    #5 0x7fc21b22c2e7 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e7)
    #6 0x57c039 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/lld+0x57c039)


--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 10.54s
********************
Failing Tests (1):
    lld :: ELF/relocatable-versioned.s

  Expected Passes    : 1881
  Unsupported Tests  : 32
  Unexpected Failures: 1
FAILED: tools/lld/test/CMakeFiles/check-lld

reverting appears to have fixed it.
The bot does not appear to be flaky.

This revision is now accepted and ready to land.Oct 23 2018, 4:23 AM
This revision was automatically updated to reflect the committed changes.

Hm, that was weird.
Avoiding truncating to i8 and then inserting into IMPLICIT_DEF reg (which was kinda ugly anyway) appears to have pacified that buildbot.
Or, there were external factors, i.e. that was flaky false-positive.