This is an archive of the discontinued LLVM Phabricator instance.

[MergeICmps] Adapt to non-eq comparisons
ClosedPublic

Authored by Allen on Jan 7 2023, 12:47 AM.

Details

Summary

Fix the last runtime issue as some sequent comparisons need be spilted.
For the origin equal comparisons chain, the new spilted Icmp chain will
still be end with equal, while for the new not-equal comparisons chain,
the new spilted Icmp chain will still be end with equal, so should address
this carefully, see detail wih case partial_sequent_ne

Diff Detail

Event Timeline

Allen created this revision.Jan 7 2023, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2023, 12:47 AM
Allen requested review of this revision.Jan 7 2023, 12:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2023, 12:47 AM
nikic added a comment.Jan 7 2023, 1:11 AM

Fix https://github.com/llvm/llvm-project/issues/59740.

This issue should remain open, because this only handles the ne case, not other predicates.

llvm/lib/Transforms/Scalar/MergeICmps.cpp
658

We should remember the correct predicate earlier and pass it in here, rather than rederiving it.

668

Builder.CreateICmp(Predicate, LhsLoad, RhsLoad)

llvm/test/Transforms/MergeICmps/X86/pr59740.ll
48

Missing negative test coverage, e.g. wrong constants in phi.

Allen updated this revision to Diff 487074.Jan 7 2023, 2:44 AM

Address comment
1、Add a new negative test cmp_ne_incorrect_const
2、Add new parameter ICmpInst::Predicate Predicate for BCECmpChain、

simplify and mergeComparisons to pass the correct predicate.

3、Update with API Builder.CreateICmp

courbet added inline comments.Jan 9 2023, 3:43 AM
llvm/lib/Transforms/Scalar/MergeICmps.cpp
843

Predicate could be a member of BCECmpChain. It's conceptually is a property of the chain and this would make the code simpler.

courbet added inline comments.Jan 9 2023, 3:57 AM
llvm/lib/Transforms/Scalar/MergeICmps.cpp
352

Let's assert that ConstBase is an i1 here, because isOne() would be incorrect otherwise.

674

nit: this is no longer IsEqual

Allen updated this revision to Diff 487410.Jan 9 2023, 6:43 AM
Allen marked 2 inline comments as done.

Address comments
1、Add member Predicate_ for BCECmpChain to make the code simpler
2、Add assert for ConstBase and Const to guard the type is i1
3、rename IsEqual to IsCmp

Allen marked 4 inline comments as done.Jan 9 2023, 6:45 AM
Allen added inline comments.
llvm/lib/Transforms/Scalar/MergeICmps.cpp
352

Apply your comment, thanks

843

Done, thanks

courbet accepted this revision.Jan 11 2023, 7:45 AM
courbet added inline comments.
llvm/lib/Transforms/Scalar/MergeICmps.cpp
644

CmpResult, or ICmpValue ?

This revision is now accepted and ready to land.Jan 11 2023, 7:45 AM
Allen updated this revision to Diff 488438.Jan 11 2023, 5:33 PM
Allen marked 2 inline comments as done.

rename IsICmp to ICmpValue

Allen marked an inline comment as done.Jan 11 2023, 5:33 PM
Allen added inline comments.
llvm/lib/Transforms/Scalar/MergeICmps.cpp
644

Done, thanks

This revision was landed with ongoing or failed builds.Jan 11 2023, 5:48 PM
This revision was automatically updated to reflect the committed changes.
Allen marked an inline comment as done.
vitalybuka reopened this revision.Jan 12 2023, 6:10 PM
vitalybuka added a subscriber: vitalybuka.

Probably breaks https://lab.llvm.org/buildbot/#/builders/85/builds/13602
The rest on blame list looks irrelevant.

This revision is now accepted and ready to land.Jan 12 2023, 6:10 PM
Allen added a comment.Jan 12 2023, 6:35 PM

Probably breaks https://lab.llvm.org/buildbot/#/builders/85/builds/13602
The rest on blame list looks irrelevant.

Thanks @vitalybuka for report, but how to reproduce the issue? I'll take a look.

Probably breaks https://lab.llvm.org/buildbot/#/builders/85/builds/13602
The rest on blame list looks irrelevant.

Thanks @vitalybuka for report, but how to reproduce the issue? I'll take a look.

precise reproducer is https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild (you need to use buildbot_bootstrap_ubsan.sh script)

but maybe just this is enough:
cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -DCMAKE_C_COMPILER=<your patched clang>/bin/clang -DCMAKE_CXX_COMPILER=<your patched clang>/bin/clang++ -DLLVM_USE_LINKER=lld '-DLLVM_ENABLE_PROJECTS='\''clang;lld;clang-tools-extra;mlir'\''' -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_SANITIZER=Undefined '-DCMAKE_C_FLAGS=-fsanitize=undefined' '-DCMAKE_CXX_FLAGS=-fsanitize=undefined' llvm-project/llvm

Allen added a comment.EditedJan 29 2023, 7:26 PM

Thanks @vitalybuka very much, I now can build the project with following command.

  • ~/scratch_dir/llvm-zorg/zorg/buildbot/builders/sanitizers/buildS/llvm-project(cbbcb10e084e) » sh ../../buildbot_bootstrap_ubsan.sh
  • base test report before the MR
Slowest Tests:
--------------------------------------------------------------------------
155.50s: Clang :: Driver/fsanitize.c
150.15s: Clang :: Driver/arm-cortex-cpus-2.c
142.50s: Clang :: Driver/arm-cortex-cpus-1.c
112.45s: Clang :: Driver/linux-ld.c
106.76s: Clang :: OpenMP/target_defaultmap_codegen_01.cpp
102.41s: Clang :: OpenMP/target_update_codegen.cpp
94.30s: Clang :: Driver/cl-options.c
74.68s: Clang :: Driver/clang_f_opts.c
73.79s: Clang :: Preprocessor/arm-target-features.c
71.09s: Clang :: Driver/riscv-arch.c
70.59s: Clang :: Driver/debug-options.c
63.96s: Clang :: Driver/x86-target-features.c
62.39s: Clang :: Preprocessor/aarch64-target-features.c
60.43s: Clang :: Preprocessor/predefined-arch-macros.c
59.80s: Clang :: Driver/response-file.c
58.29s: Clang :: Driver/aarch64-fp16.c
55.18s: Clang :: Driver/cl-outputs.c
53.77s: Clang :: OpenMP/target_data_codegen.cpp
50.78s: Clang :: Driver/pic.c
49.10s: lit :: shtest-define.py
Tests Times:
--------------------------------------------------------------------------
[   Range   ] :: [               Percentage               ] :: [   Count   ]
--------------------------------------------------------------------------
[150s,160s) :: [                                        ] :: [    2/80132]
[140s,150s) :: [                                        ] :: [    1/80132]
[130s,140s) :: [                                        ] :: [    0/80132]
[120s,130s) :: [                                        ] :: [    0/80132]
[110s,120s) :: [                                        ] :: [    1/80132]
[100s,110s) :: [                                        ] :: [    2/80132]
[ 90s,100s) :: [                                        ] :: [    1/80132]
[ 80s, 90s) :: [                                        ] :: [    0/80132]
[ 70s, 80s) :: [                                        ] :: [    4/80132]
[ 60s, 70s) :: [                                        ] :: [    4/80132]
[ 50s, 60s) :: [                                        ] :: [    5/80132]
[ 40s, 50s) :: [                                        ] :: [   10/80132]
[ 30s, 40s) :: [                                        ] :: [   24/80132]
[ 20s, 30s) :: [                                        ] :: [   48/80132]
[ 10s, 20s) :: [                                        ] :: [  271/80132]
[  0s, 10s) :: [*************************************** ] :: [79759/80132]
--------------------------------------------------------------------------
Testing Time: 583.25s
  Skipped          :    52
  Unsupported      :   975
  Passed           : 87224
  Expectedly Failed:   191
  • With the MR, it also pass, but only taking some more times
Slowest Tests:
--------------------------------------------------------------------------
155.65s: Clang :: Driver/fsanitize.c
146.70s: Clang :: Driver/arm-cortex-cpus-2.c
130.69s: Clang :: Driver/arm-cortex-cpus-1.c
104.99s: Clang :: Driver/linux-ld.c
93.88s: Clang :: OpenMP/target_update_codegen.cpp
93.66s: Clang :: OpenMP/target_defaultmap_codegen_01.cpp
84.77s: Clang :: Driver/clang_f_opts.c
81.02s: Clang :: Driver/cl-options.c
67.31s: Clang :: Driver/debug-options.c
65.48s: Clang :: Preprocessor/arm-target-features.c
64.31s: Clang :: Driver/riscv-arch.c
61.93s: Clang :: Driver/mips-fsf.cpp
61.89s: Clang :: Preprocessor/aarch64-target-features.c
59.26s: Clang :: Preprocessor/predefined-arch-macros.c
58.48s: lit :: shtest-define.py
54.39s: lit :: shtest-shell.py
50.12s: Clang :: OpenMP/target_data_codegen.cpp
48.99s: LLVM :: CodeGen/ARM/build-attributes.ll
46.24s: Clang :: Driver/x86-target-features.c
46.18s: Clang :: Driver/cl-outputs.c
Tests Times:
--------------------------------------------------------------------------
[   Range   ] :: [               Percentage               ] :: [   Count   ]
--------------------------------------------------------------------------
[150s,160s) :: [                                        ] :: [    2/80056]
[140s,150s) :: [                                        ] :: [    0/80056]
[130s,140s) :: [                                        ] :: [    1/80056]
[120s,130s) :: [                                        ] :: [    0/80056]
[110s,120s) :: [                                        ] :: [    0/80056]
[100s,110s) :: [                                        ] :: [    1/80056]
[ 90s,100s) :: [                                        ] :: [    2/80056]
[ 80s, 90s) :: [                                        ] :: [    2/80056]
[ 70s, 80s) :: [                                        ] :: [    0/80056]
[ 60s, 70s) :: [                                        ] :: [    7/80056]
[ 50s, 60s) :: [                                        ] :: [    3/80056]
[ 40s, 50s) :: [                                        ] :: [   14/80056]
[ 30s, 40s) :: [                                        ] :: [   18/80056]
[ 20s, 30s) :: [                                        ] :: [   66/80056]
[ 10s, 20s) :: [                                        ] :: [  248/80056]
[  0s, 10s) :: [*************************************** ] :: [79692/80056]
--------------------------------------------------------------------------
Testing Time: 626.41s
  Skipped          :    52
  Unsupported      :   975
  Passed           : 87225
  Expectedly Failed:   191
Allen added a comment.Feb 1 2023, 12:02 AM

hi @vitalybuka, Excuse me,again.

May the issue relate to the timeout or I miss somethings to reprocduce the break ?
Allen added a comment.Feb 20 2023, 8:32 AM

hi @vitalybuka, as the issue can't be repoduced locally, do you object to retry again ?

vitalybuka accepted this revision.Feb 26 2023, 12:36 AM

hi @vitalybuka, as the issue can't be repoduced locally, do you object to retry again ?

I can't reproduce it as well.

This revision was automatically updated to reflect the committed changes.
bgraur added a subscriber: bgraur.Feb 28 2023, 10:06 AM
bgraur added a comment.Mar 1 2023, 8:11 AM

@Allen test unittests/ProfileData/ProfileDataTests fails when built with optimisations level -O1 with clang including this patch.

When building the test with -O3 (Release) or -O1 -g it passes.

Not sure if this patch triggers some other bug in clang or the test has UB or the patch itself is to blame.
Please revert this patch and investigate the issue without any pressure.

Repro steps:

  1. Build and install clang somewhere.
  2. Build the test with ninja ProfileDataTests -v. The compilation command and linker command will be displayed.
  3. Edit the commands to specify -O1 and no -g. It is enough to only build the test in unittests/ProfileData/CoverageMappingTest.cpp as that one shows the failure.

The command will look something like this:

${INSTALL_PATH}/bin/clang++ \
  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS \
  -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS \
  -I${PROJECT}/llvm-project/build/unittests/ProfileData \
  -I${PROJECT}/llvm-project/llvm/unittests/ProfileData \
  -I${PROJECT}/llvm-project/build/include \
  -I${PROJECT}/llvm-project/llvm/include \
  -I${PROJECT}/llvm-project/third-party/unittest/googletest/include \
  -I${PROJECT}/llvm-project/third-party/unittest/googlemock/include \
  -O1 -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new \
  -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long \
  -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor \
  -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported \
  -fdiagnostics-color -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti \
  -Wno-suggest-override -std=c++17 \
  -MD -MT unittests/ProfileData/CMakeFiles/ProfileDataTests.dir/CoverageMappingTest.cpp.o \
  -MF unittests/ProfileData/CMakeFiles/ProfileDataTests.dir/CoverageMappingTest.cpp.o.d \
  -o unittests/ProfileData/CMakeFiles/ProfileDataTests.dir/CoverageMappingTest.cpp.o \
  -c ${PROJECT}/llvm-project/llvm/unittests/ProfileData/CoverageMappingTest.cpp

${INSTALL_PATH}/bin/clang++ -O1 -fPIC -fno-semantic-interposition \
  -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter \
  -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi \
  -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor \
  -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color \
  unittests/ProfileData/CMakeFiles/ProfileDataTests.dir/CoverageMappingTest.cpp.o \
  -o unittests/ProfileData/ProfileDataTests  \
  lib/libLLVMCore.a  lib/libLLVMCoverage.a  lib/libLLVMProfileData.a  lib/libLLVMSupport.a  lib/libLLVMObject.a  \
  lib/libLLVMSupport.a  lib/libllvm_gtest_main.a  lib/libllvm_gtest.a  lib/libLLVMTestingSupport.a  \
  lib/libLLVMSymbolize.a  lib/libLLVMDebugInfoPDB.a  lib/libLLVMDebugInfoMSF.a  lib/libLLVMDebugInfoDWARF.a  \
  lib/libLLVMObject.a  lib/libLLVMIRReader.a  lib/libLLVMBitReader.a  lib/libLLVMAsmParser.a  lib/libLLVMCore.a  \
  lib/libLLVMRemarks.a  lib/libLLVMBitstreamReader.a  lib/libLLVMMCParser.a  lib/libLLVMMC.a  lib/libLLVMDebugInfoCodeView.a  \
  lib/libLLVMTextAPI.a  lib/libLLVMBinaryFormat.a  lib/libLLVMTargetParser.a  lib/libllvm_gtest.a  lib/libLLVMSupport.a  \
  lib/libLLVMDemangle.a  -lrt  -ldl  -lm  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libtinfo.so  -lpthread 

$ unittests/ProfileData/ProfileDataTests
..........................
[==========] 118 tests from 2 test suites ran. (4 ms total)
[  PASSED  ] 114 tests.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] ParameterizedCovMapTest/CoverageMappingTest.test_line_coverage_iterator/0, where GetParam() = (false, false)
[  FAILED  ] ParameterizedCovMapTest/CoverageMappingTest.test_line_coverage_iterator/1, where GetParam() = (false, true)
[  FAILED  ] ParameterizedCovMapTest/CoverageMappingTest.test_line_coverage_iterator/2, where GetParam() = (true, false)
[  FAILED  ] ParameterizedCovMapTest/CoverageMappingTest.test_line_coverage_iterator/3, where GetParam() = (true, true)
Allen added a comment.Mar 1 2023, 5:13 PM

@Allen test unittests/ProfileData/ProfileDataTests fails when built with optimisations level -O1 with clang including this patch.

When building the test with -O3 (Release) or -O1 -g it passes.

Not sure if this patch triggers some other bug in clang or the test has UB or the patch itself is to blame.
Please revert this patch and investigate the issue without any pressure.

Thanks for your report, I'll take a look at that and now revert with commit af2969fd46b7

ayzhao added a subscriber: ayzhao.Mar 2 2023, 3:19 PM

FYI this patch also caused some of Chrome's unit tests to fail - see https://crbug.com/1420824

Allen added a comment.EditedMar 6 2023, 4:56 AM

For the runtime fails of unittests/ProfileData/ProfileDataTests, there is 2 points important

  • a) build without -g fall as it transform code (From function _ZN12_GLOBAL__N_152CoverageMappingTest_test_line_coverage_iterator_Test8TestBodyEv)
_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE3endEv.exit: ; preds = %_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE5beginEv.exit, %if.then.i.i.i294
  %Stats.i.i295 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 6
  %Stats4.i.i296 = getelementptr inbounds %"class.llvm::iterator_range", ptr %ref.tmp32, i64 0, i32 1, i32 6
  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(40) %Stats.i.i295, ptr noundef nonnull align 8 dereferenceable(40) %Stats4.i.i296, i64 40, i1 false), !tbaa.struct !339
  %Next.i.i299 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 2
  %Next3.i.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 2
  %Ended.i.i301 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 3
  %Ended4.i.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 3
  %Line.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 6, i32 3
  %message_.i320 = getelementptr inbounds %"class.testing::AssertionResult", ptr %gtest_ar35, i64 0, i32 1
  %message_.i386 = getelementptr inbounds %"class.testing::AssertionResult", ptr %gtest_ar55, i64 0, i32 1
  br label %for.cond

for.cond:                                         ; preds = %_ZN7testing15AssertionResultD2Ev.exit392, %_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE3endEv.exit
  %340 = load ptr, ptr %__begin1, align 8, !tbaa !340
  %341 = load ptr, ptr %__end1, align 8, !tbaa !340
  %cmp.i.i298 = icmp eq ptr %340, %341
  br i1 %cmp.i.i298, label %land.lhs.true.i.i, label %_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit

land.lhs.true.i.i:                                ; preds = %for.cond
  %342 = load ptr, ptr %Next.i.i299, align 8, !tbaa !48
  %343 = load ptr, ptr %Next3.i.i, align 8, !tbaa !48
  %cmp.i.i.i300 = icmp eq ptr %342, %343
  br i1 %cmp.i.i.i300, label %land.rhs.i.i, label %_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit

land.rhs.i.i:                                     ; preds = %land.lhs.true.i.i
  %344 = load i8, ptr %Ended.i.i301, align 8, !tbaa !333, !range !18, !noundef !19
  %345 = load i8, ptr %Ended4.i.i, align 8, !tbaa !333, !range !18, !noundef !19
  %cmp7.i.i = icmp ne i8 %344, %345
  br label %_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit

_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit: ; preds = %for.cond, %land.lhs.true.i.i, %land.rhs.i.i
  %lnot.i = phi i1 [ true, %land.lhs.true.i.i ], [ true, %for.cond ], [ %cmp7.i.i, %land.rhs.i.i ]
  br i1 %lnot.i, label %for.body, label %for.end.critedge

into a %memcmp, and the detail code shows as following (The first %340 = icmp ne ptr %338, %339 should be keep using condition eq )

_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE3endEv.exit: ; preds = %_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE5beginEv.exit, %if.then.i.i.i294
  %Stats.i.i295 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 6
  %Stats4.i.i296 = getelementptr inbounds %"class.llvm::iterator_range", ptr %ref.tmp32, i64 0, i32 1, i32 6
  call void @llvm.memcpy.p0.p0.i64(ptr noundef nonnull align 8 dereferenceable(40) %Stats.i.i295, ptr noundef nonnull align 8 dereferenceable(40) %Stats4.i.i296, i64 40, i1 false), !tbaa.struct !339
  %Next.i.i299 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 2
  %Next3.i.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 2
  %Ended.i.i301 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 3
  %Ended4.i.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 3
  %Line.i = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 6, i32 3
  %message_.i320 = getelementptr inbounds %"class.testing::AssertionResult", ptr %gtest_ar35, i64 0, i32 1
  %message_.i386 = getelementptr inbounds %"class.testing::AssertionResult", ptr %gtest_ar55, i64 0, i32 1
  br label %for.cond582

for.cond582:                                      ; preds = %_ZNK4llvm14iterator_rangeINS_8coverage20LineCoverageIteratorEE3endEv.exit, %_ZN7testing15AssertionResultD2Ev.exit392
  %338 = load ptr, ptr %__begin1, align 8
  %339 = load ptr, ptr %__end1, align 8
  %340 = icmp ne ptr %338, %339
  br i1 %340, label %"land.lhs.true.i.i+land.rhs.i.i", label %_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit

"land.lhs.true.i.i+land.rhs.i.i":                 ; preds = %for.cond582
  %341 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__begin1, i64 0, i32 2
  %342 = getelementptr inbounds %"class.llvm::coverage::LineCoverageIterator", ptr %__end1, i64 0, i32 2
  %memcmp = call i32 @memcmp(ptr %341, ptr %342, i64 9)
  %343 = icmp ne i32 %memcmp, 0
  br label %_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit

_ZNK4llvm20iterator_facade_baseINS_8coverage20LineCoverageIteratorESt20forward_iterator_tagKNS1_17LineCoverageStatsElPS5_RS5_EneERKS2_.exit: ; preds = %for.cond582, %"land.lhs.true.i.i+land.rhs.i.i"
  %lnot.i = phi i1 [ %343, %"land.lhs.true.i.i+land.rhs.i.i" ], [ false, %for.cond582 ]
  br i1 %lnot.i, label %for.body, label %for.end.critedge
  1. b) build with -g works fine is because Comparison->doesOtherWork() in function BCECmpChain::BCECmpChain return false (the transform doesn't active) as some call void @llvm.dbg.value, which is not exist when build without -g.
Allen updated this revision to Diff 502970.Mar 7 2023, 2:51 AM
Allen added reviewers: bgraur, ayzhao.

Fix the runtime issue related to test unittests/ProfileData/ProfileDataTests.

The issue also can be reproduced on aarch64 cpu, the failure is caused by
the new spilted Icmp chain from not-equal comparisons chain is end with
equal condition, I am not aware of this before.

Allen added a comment.Mar 7 2023, 2:53 AM

FYI this patch also caused some of Firefox's unit tests to fail, e.g. https://treeherder.mozilla.org/logviewer?job_id=407511605&repo=toolchains&lineNumber=32470

hi @glandium, As I don't know how to check the above tests, would you help me to check that base on my new update changes ?

ayzhao added a comment.Mar 8 2023, 2:38 PM

I tested the updated version of this patch to see if it fixes the Chrome test failures. Unfortunately, Clang now crashes when compiling an unrelated file in Chrome:

[5373/37986] CXX obj/third_party/angle/translator/ConvertUnsupportedConstructorsToFuncti[11272/37597] CXX obj/skia/skia/GrQuadUtils.o                                           FAILED: obj/skia/skia/GrQuadUtils.o
/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang++ -MMD -MF obj/skia/skia/GrQuadUtils.o.d -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUSE_AURA=1 -DUSE_GLIB=1 -DUSE_OZONE=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -DCR_LIBCXX_REVISION=3cf0fe4445843426d7c51eca9daad9ae0c2ff77d -D_LIBCPP_ENABLE_ASSERTIONS=1 -DCR_SYSROOT_KEY=20221105T211506Z-0 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DSK_CODEC_DECODES_PNG -DSK_CODEC_DECODES_WEBP -DSK_ENCODE_PNG -DSK_ENCODE_WEBP -DSK_ENABLE_SKSL -DSK_UNTIL_CRBUG_1187654_IS_FIXED -DSK_USER_CONFIG_HEADER=\"../../skia/config/SkUserConfig.h\" -DSK_WIN_FONTMGR_NO_SIMULATIONS -DSK_GL -DSK_CODEC_DECODES_JPEG -DSK_ENCODE_JPEG -DSK_HAS_WUFFS_LIBRARY -DSK_VULKAN=1 -DSK_GANESH -DSK_GPU_WORKAROUNDS_HEADER=\"gpu/config/gpu_driver_bug_workaround_autogen.h\" -DVK_USE_PLATFORM_XCB_KHR -DVK_USE_PLATFORM_WAYLAND_KHR -DIS_SKIA_IMPL=1 -DSKIA_IMPLEMENTATION=1 -DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=\(\(\(FREETYPE_MAJOR\)\ \*\ 0x01000000\)\ \|\ \(\(FREETYPE_MINOR\)\ \*\ 0x00010000\)\ \|\ \(\(FREETYPE_PATCH\)\ \*\ 0x00000100\)\) -DSK_TYPEFACE_FACTORY_FREETYPE -DSK_GAMMA_EXPONENT=1.2 -DSK_GAMMA_CONTRAST=0.2 -DSK_DEFAULT_FONT_CACHE_LIMIT=20971520 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_56 -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_56 -DWEBP_EXTERN=extern -DFT_CONFIG_MODULES_H=\"freetype-custom/freetype/config/ftmodule.h\" -DFT_CONFIG_OPTIONS_H=\"freetype-custom/freetype/config/ftoption.h\" -DPDFIUM_REQUIRED_MODULES -DCHROMIUM_RESTRICT_VISIBILITY -DUSE_LIBJPEG_TURBO=1 -DMANGLE_JPEG_NAMES -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_ENABLE_TRACING=1 -DU_ENABLE_RESOURCE_TRACING=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen -I../../buildtools/third_party/libc++ -I../../third_party/skia -I../../third_party/wuffs/src/release/c -I../../third_party/vulkan/include -I../../third_party/vulkan-deps/vulkan-headers/src/include -I../../third_party/wayland/src/src -I../../third_party/wayland/include/src -I../../third_party/perfetto/include -Igen/third_party/perfetto/build_config -Igen/third_party/perfetto -I../../third_party/libwebp/src/src -I../../third_party/abseil-cpp -I../../third_party/boringssl/src/include -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/freetype/include -I../../third_party/freetype/include/freetype-custom -I../../third_party/freetype/src/include -I../../third_party/harfbuzz-ng/src/src -I../../third_party/libjpeg_turbo -I../../third_party/fontconfig/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fno-delete-null-pointer-checks -fno-ident -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pthread -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -mllvm -instcombine-lower-dbg-declare=0 -ffp-contract=off -fcomplete-member-pointers -m64 -msse3 -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -ffile-compilation-dir=. -no-canonical-prefixes -ftrivial-auto-var-init=pattern -fno-omit-frame-pointer -g0 -fvisibility=hidden -Wno-redundant-parens -Werror -Wall -Wno-unused-variable -Wno-c++11-narrowing -Wno-unused-but-set-variable -Wno-misleading-indentation -Wno-missing-field-initializers -Wno-unused-parameter -Wno-psabi -Wloop-analysis -Wno-unneeded-internal-declaration -Wenum-compare-conditional -Wno-ignored-pragma-optimize -Wno-deprecated-builtins -Wno-bitfield-constant-conversion -Wno-deprecated-this-capture -O2 -fdata-sections -ffunction-sections -fno-unique-section-names -isystem../../build/linux/debian_bullseye_amd64-sysroot/usr/include/glib-2.0 -isystem../../build/linux/debian_bullseye_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -DPROTOBUF_ALLOW_DEPRECATED=1 -std=c++20 -Wno-trigraphs -gsimple-template-names -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../build/linux/debian_bullseye_amd64-sysroot -fvisibility-inlines-hidden -c ../../third_party/skia/src/gpu/ganesh/geometry/GrQuadUtils.cpp -o obj/skia/skia/GrQuadUtils.o
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: /usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang++ -MMD -MF obj/skia/skia/GrQuadUtils.o.d -DDCHECK_ALWAYS_ON=1 -DUSE_UDEV -DUSE_AURA=1 -DUSE_GLIB=1 -DUSE_OZONE=1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCXXABI_DISABLE_VISIBILITY_ANNOTATIONS -DCR_LIBCXX_REVISION=3cf0fe4445843426d7c51eca9daad9ae0c2ff77d -D_LIBCPP_ENABLE_ASSERTIONS=1 -DCR_SYSROOT_KEY=20221105T211506Z-0 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -DSK_CODEC_DECODES_PNG -DSK_CODEC_DECODES_WEBP -DSK_ENCODE_PNG -DSK_ENCODE_WEBP -DSK_ENABLE_SKSL -DSK_UNTIL_CRBUG_1187654_IS_FIXED -DSK_USER_CONFIG_HEADER=\"../../skia/config/SkUserConfig.h\" -DSK_WIN_FONTMGR_NO_SIMULATIONS -DSK_GL -DSK_CODEC_DECODES_JPEG -DSK_ENCODE_JPEG -DSK_HAS_WUFFS_LIBRARY -DSK_VULKAN=1 -DSK_GANESH -DSK_GPU_WORKAROUNDS_HEADER=\"gpu/config/gpu_driver_bug_workaround_autogen.h\" -DVK_USE_PLATFORM_XCB_KHR -DVK_USE_PLATFORM_WAYLAND_KHR -DIS_SKIA_IMPL=1 -DSKIA_IMPLEMENTATION=1 "-DSK_FREETYPE_MINIMUM_RUNTIME_VERSION=(((FREETYPE_MAJOR) * 0x01000000) | ((FREETYPE_MINOR) * 0x00010000) | ((FREETYPE_PATCH) * 0x00000100))" -DSK_TYPEFACE_FACTORY_FREETYPE -DSK_GAMMA_EXPONENT=1.2 -DSK_GAMMA_CONTRAST=0.2 -DSK_DEFAULT_FONT_CACHE_LIMIT=20971520 -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_56 -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_56 -DWEBP_EXTERN=extern -DFT_CONFIG_MODULES_H=\"freetype-custom/freetype/config/ftmodule.h\" -DFT_CONFIG_OPTIONS_H=\"freetype-custom/freetype/config/ftoption.h\" -DPDFIUM_REQUIRED_MODULES -DCHROMIUM_RESTRICT_VISIBILITY -DUSE_LIBJPEG_TURBO=1 -DMANGLE_JPEG_NAMES -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DUSE_CHROMIUM_ICU=1 -DU_ENABLE_TRACING=1 -DU_ENABLE_RESOURCE_TRACING=0 -DU_STATIC_IMPLEMENTATION -DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE -I../.. -Igen -I../../buildtools/third_party/libc++ -I../../third_party/skia -I../../third_party/wuffs/src/release/c -I../../third_party/vulkan/include -I../../third_party/vulkan-deps/vulkan-headers/src/include -I../../third_party/wayland/src/src -I../../third_party/wayland/include/src -I../../third_party/perfetto/include -Igen/third_party/perfetto/build_config -Igen/third_party/perfetto -I../../third_party/libwebp/src/src -I../../third_party/abseil-cpp -I../../third_party/boringssl/src/include -I../../third_party/protobuf/src -Igen/protoc_out -I../../third_party/libpng -I../../third_party/zlib -I../../third_party/freetype/include -I../../third_party/freetype/include/freetype-custom -I../../third_party/freetype/src/include -I../../third_party/harfbuzz-ng/src/src -I../../third_party/libjpeg_turbo -I../../third_party/fontconfig/src -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -fno-delete-null-pointer-checks -fno-ident -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pthread -fcolor-diagnostics -fmerge-all-constants -fcrash-diagnostics-dir=../../tools/clang/crashreports -mllvm -instcombine-lower-dbg-declare=0 -ffp-contract=off -fcomplete-member-pointers -m64 -msse3 -Wno-builtin-macro-redefined -D__DATE__= -D__TIME__= -D__TIMESTAMP__= -ffile-compilation-dir=. -no-canonical-prefixes -ftrivial-auto-var-init=pattern -fno-omit-frame-pointer -g0 -fvisibility=hidden -Wno-redundant-parens -Werror -Wall -Wno-unused-variable -Wno-c++11-narrowing -Wno-unused-but-set-variable -Wno-misleading-indentation -Wno-missing-field-initializers -Wno-unused-parameter -Wno-psabi -Wloop-analysis -Wno-unneeded-internal-declaration -Wenum-compare-conditional -Wno-ignored-pragma-optimize -Wno-deprecated-builtins -Wno-bitfield-constant-conversion -Wno-deprecated-this-capture -O2 -fdata-sections -ffunction-sections -fno-unique-section-names -isystem../../build/linux/debian_bullseye_amd64-sysroot/usr/include/glib-2.0 -isystem../../build/linux/debian_bullseye_amd64-sysroot/usr/lib/x86_64-linux-gnu/glib-2.0/include -DPROTOBUF_ALLOW_DEPRECATED=1 -std=c++20 -Wno-trigraphs -gsimple-template-names -fno-exceptions -fno-rtti -nostdinc++ -isystem../../buildtools/third_party/libc++/trunk/include -isystem../../buildtools/third_party/libc++abi/trunk/include --sysroot=../../build/linux/debian_bullseye_amd64-sysroot -fvisibility-inlines-hidden -c ../../third_party/skia/src/gpu/ganesh/geometry/GrQuadUtils.cpp -o obj/skia/skia/GrQuadUtils.o
1.      <eof> parser at end of file
2.      Optimizer
 #0 0x000055b398f7c297 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3ef5297)
 #1 0x000055b398f7a14e llvm::sys::RunSignalHandlers() (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3ef314e)
 #2 0x000055b398ef7d48 CrashRecoverySignalHandler(int) CrashRecoveryContext.cpp:0:0
 #3 0x00007fc93b45af90 (/lib/x86_64-linux-gnu/libc.so.6+0x3bf90)
 #4 0x000055b397d6606e llvm::DataLayout::getTypeSizeInBits(llvm::Type*) const X86ISelLowering.cpp:0:0
 #5 0x000055b398e619d5 llvm::SROAPass::rewritePartition(llvm::AllocaInst&, llvm::sroa::AllocaSlices&, llvm::sroa::Partition&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3dda9d5)
 #6 0x000055b398e65788 llvm::SROAPass::splitAlloca(llvm::AllocaInst&, llvm::sroa::AllocaSlices&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3dde788)
 #7 0x000055b398e68e21 llvm::SROAPass::runOnAlloca(llvm::AllocaInst&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3de1e21)
 #8 0x000055b398e6c19e llvm::SROAPass::runImpl(llvm::Function&, llvm::DomTreeUpdater&, llvm::AssumptionCache&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3de519e)
 #9 0x000055b398e6d361 llvm::SROAPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3de6361)
#10 0x000055b39a38883d llvm::detail::PassModel<llvm::Function, llvm::SROAPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) PassBuilder.cpp:0:0
#11 0x000055b3989e725e llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x396025e)
#12 0x000055b39984522d llvm::detail::PassModel<llvm::Function, llvm::PassManager<llvm::Function, llvm::AnalysisManager<llvm::Function>>, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Function>>::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&) BackendUtil.cpp:0:0
#13 0x000055b398a7a0c8 llvm::CGSCCToFunctionPassAdaptor::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x39f30c8)
#14 0x000055b39a38a29d llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::CGSCCToFunctionPassAdaptor, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) PassBuilder.cpp:0:0
#15 0x000055b398a74bc4 llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x39edbc4)
#16 0x000055b398a97c0d llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::PassManager<llvm::LazyCallGraph::SCC, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) Inliner.cpp:0:0
#17 0x000055b398a782a9 llvm::DevirtSCCRepeatedPass::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x39f12a9)
#18 0x000055b398a9844d llvm::detail::PassModel<llvm::LazyCallGraph::SCC, llvm::DevirtSCCRepeatedPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&>::run(llvm::LazyCallGraph::SCC&, llvm::AnalysisManager<llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&>&, llvm::LazyCallGraph&, llvm::CGSCCUpdateResult&) Inliner.cpp:0:0
#19 0x000055b398a76dca llvm::ModuleToPostOrderCGSCCPassAdaptor::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x39efdca)
#20 0x000055b398a981ed llvm::detail::PassModel<llvm::Module, llvm::ModuleToPostOrderCGSCCPassAdaptor, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) Inliner.cpp:0:0
#21 0x000055b3989e5eae llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x395eeae)
#22 0x000055b398a93afc llvm::ModuleInlinerWrapperPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3a0cafc)
#23 0x000055b39a37a62d llvm::detail::PassModel<llvm::Module, llvm::ModuleInlinerWrapperPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) PassBuilder.cpp:0:0
#24 0x000055b3989e5eae llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x395eeae)
#25 0x000055b39983d5d9 (anonymous namespace)::EmitAssemblyHelper::RunOptimizationPipeline(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream>>&, std::unique_ptr<llvm::ToolOutputFile, std::default_delete<llvm::ToolOutputFile>>&) BackendUtil.cpp:0:0
#26 0x000055b399834371 clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::StringRef, llvm::Module*, clang::BackendAction, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream>>) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x47ad371)
#27 0x000055b399ccb174 clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) CodeGenAction.cpp:0:0
#28 0x000055b39b255f24 clang::ParseAST(clang::Sema&, bool, bool) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x61cef24)
#29 0x000055b399be2800 clang::FrontendAction::Execute() (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x4b5b800)
#30 0x000055b399b4e09f clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x4ac709f)
#31 0x000055b399cc3f8f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x4c3cf8f)
#32 0x000055b397cef733 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x2c68733)
#33 0x000055b397ceb4e1 ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&, llvm::ToolContext const&) driver.cpp:0:0
#34 0x000055b3999c6e29 void llvm::function_ref<void ()>::callback_fn<clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const::$_1>(long) Job.cpp:0:0
#35 0x000055b398ef7a8b llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x3e70a8b)
#36 0x000055b3999c6280 clang::driver::CC1Command::Execute(llvm::ArrayRef<std::optional<llvm::StringRef>>, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, bool*) const (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x493f280)
#37 0x000055b399984dde clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, clang::driver::Command const*&, bool) const (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x48fddde)
#38 0x000055b399985097 clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&, bool) const (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x48fe097)
#39 0x000055b3999a4919 clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::pair<int, clang::driver::Command const*>>&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x491d919)
#40 0x000055b397ceaa58 clang_main(int, char**, llvm::ToolContext const&) (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x2c63a58)
#41 0x000055b397cfbed1 main (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x2c74ed1)
#42 0x00007fc93b44618a __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3
#43 0x00007fc93b446245 call_init ./csu/../csu/libc-start.c:128:20
#44 0x00007fc93b446245 __libc_start_main ./csu/../csu/libc-start.c:368:5
#45 0x000055b397ce7521 _start (/usr/local/google/home/ayzhao/src/llvm-project/build/bin/clang+++0x2c60521)
clang++: error: clang frontend command failed with exit code 139 (use -v to see invocation)
clang version 17.0.0 (git@github.com:llvm/llvm-project.git 55d5410442f446e4edf1c4ba850d24631b7b6e80)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/google/home/ayzhao/src/llvm-project/build/bin
clang++: note: diagnostic msg:
********************

PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang++: note: diagnostic msg: ../../tools/clang/crashreports/GrQuadUtils-ab2f11.cpp
clang++: note: diagnostic msg: ../../tools/clang/crashreports/GrQuadUtils-ab2f11.sh
clang++: note: diagnostic msg:

********************
[11401/37597] CXX obj/skia/skia/GrGLGpu.o
ninja: build stopped: subcommand failed.

File in question: https://crsrc.org/c/third_party/skia/src/gpu/ganesh/geometry/GrQuadUtils.cpp

FYI this patch also caused some of Firefox's unit tests to fail, e.g. https://treeherder.mozilla.org/logviewer?job_id=407511605&repo=toolchains&lineNumber=32470

hi @glandium, As I don't know how to check the above tests, would you help me to check that base on my new update changes ?

Building Firefox with this new patch applied yields a clang crash like @ayzhao's.

Allen added a comment.Mar 8 2023, 8:47 PM

Thanks @glandium and @ayzhao, I'm trying to reproduce the above issue with chromium's code

The crash is actually happening without the patch too. It happens both in Firefox and Chrome because both are using Skia, and the source that triggers the crash is in Skia.

Allen added a comment.Mar 8 2023, 11:04 PM

The crash comes from D143225.

Oh, thanks @glandium very much for your information.
In that case, I'll land this patch in the near future, as the previous runtime issue has been resolved.

The crash comes from D143225.

Oh, thanks @glandium very much for your information.
In that case, I'll land this patch in the near future, as the previous runtime issue has been resolved.

I'm testing it against trunk minus the patch that causes the crash. In a couple hours or less, I'll be able to tell you whether the latest version or the patch works properly for Firefox.

I'm testing it against trunk minus the patch that causes the crash. In a couple hours or less, I'll be able to tell you whether the latest version or the patch works properly for Firefox.

It seems good. Thanks.

Allen added a comment.Mar 9 2023, 2:47 AM

Thank for your carefully examine.

ayzhao added a comment.Mar 9 2023, 2:36 PM

I'm continuing to see the same unit test failure as https://crbug.com/1420824#c2 after including the latest commit

Allen added a comment.Mar 9 2023, 5:12 PM

I'm continuing to see the same unit test failure as https://crbug.com/1420824#c2 after including the latest commit

Are the symptoms and reproduction methods consistent with those mentioned earlier? If so, I'll also try to analyze it based on the latest commit.

miscompile:

$ cat a.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"                                                                                                       
target triple = "x86_64-unknown-linux-gnu"                                                                                                                                                         
                                                                                                                                                                                                   
%"struct.media::WebrtcVideoStatsDB::VideoDescKey" = type { i8, i32, i8, i32 }                                                                                                                      
                                                                                                                                                                                                   
define i1 @_ZN5medianeERKNS_18WebrtcVideoStatsDB12VideoDescKeyES3_(ptr nocapture noundef readonly align 4 dereferenceable(16) %0, ptr nocapture noundef readonly align 4 dereferenceable(16) %1) { 
  %3 = load i8, ptr %0, align 4, !range !4, !noundef !5
  %4 = load i8, ptr %1, align 4, !range !4, !noundef !5
  %5 = icmp eq i8 %3, %4
  br i1 %5, label %6, label %24

6:                                                ; preds = %2
  %7 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %0, i64 0, i32 1
  %8 = load i32, ptr %7, align 4
  %9 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %1, i64 0, i32 1
  %10 = load i32, ptr %9, align 4
  %11 = icmp eq i32 %8, %10
  br i1 %11, label %12, label %24

12:                                               ; preds = %6
  %13 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %0, i64 0, i32 2
  %14 = load i8, ptr %13, align 4, !range !4, !noundef !5
  %15 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %1, i64 0, i32 2
  %16 = load i8, ptr %15, align 4, !range !4, !noundef !5
  %17 = icmp eq i8 %14, %16
  br i1 %17, label %18, label %24

18:                                               ; preds = %12
  %19 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %0, i64 0, i32 3
  %20 = load i32, ptr %19, align 4
  %21 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %1, i64 0, i32 3
  %22 = load i32, ptr %21, align 4
  %23 = icmp ne i32 %20, %22
  br label %24

24:                                               ; preds = %18, %12, %6, %2
  %25 = phi i1 [ true, %12 ], [ true, %6 ], [ true, %2 ], [ %23, %18 ]
  ret i1 %25
}

!4 = !{i8 0, i8 2}
!5 = !{}
$ opt -passes=mergeicmps -S a.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%"struct.media::WebrtcVideoStatsDB::VideoDescKey" = type { i8, i32, i8, i32 }

define i1 @_ZN5medianeERKNS_18WebrtcVideoStatsDB12VideoDescKeyES3_(ptr nocapture noundef readonly align 4 dereferenceable(16) %0, ptr nocapture noundef readonly align 4 dereferenceable(16) %1) {
  %3 = load i8, ptr %0, align 1
  %4 = load i8, ptr %1, align 1
  %5 = icmp eq i8 %3, %4
  br i1 %5, label %6, label %16

6:                                                ; preds = %2
  %7 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %0, i64 0, i32 1
  %8 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %1, i64 0, i32 1
  %memcmp = call i32 @memcmp(ptr %7, ptr %8, i64 5)
  %9 = icmp eq i32 %memcmp, 0
  br i1 %9, label %10, label %16

10:                                               ; preds = %6
  %11 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %0, i64 0, i32 3
  %12 = getelementptr inbounds %"struct.media::WebrtcVideoStatsDB::VideoDescKey", ptr %1, i64 0, i32 3
  %13 = load i32, ptr %11, align 4
  %14 = load i32, ptr %12, align 4
  %15 = icmp ne i32 %13, %14
  br label %16

16:                                               ; preds = %2, %6, %10
  %17 = phi i1 [ %15, %10 ], [ false, %6 ], [ false, %2 ]
  ret i1 %17
}

; Function Attrs: nofree nounwind willreturn memory(argmem: read)
declare i32 @memcmp(ptr nocapture, ptr nocapture, i64) #0

attributes #0 = { nofree nounwind willreturn memory(argmem: read) }

the phi value from the entry block turns from true to false
(also the metadata for the loads in the entry block don't need to be dropped?)

will revert this patch

Allen added a comment.EditedMar 23 2023, 2:10 AM

hi @aeubanks, I try to address above metadata issue on D146702.

Allen added a comment.EditedApr 7 2023, 8:41 PM

hi @aeubanks, I recheck your above IR base on alive2, and it seems the transform is right ? see detail https://alive2.llvm.org/ce/z/GMHxjL

Allen reopened this revision.Apr 7 2023, 11:24 PM
This revision is now accepted and ready to land.Apr 7 2023, 11:24 PM

unless I'm overlooking something, the [ true, %2 ] -> [ false, %2 ] is pretty clearly wrong

Allen added a comment.Apr 9 2023, 6:31 PM

unless I'm overlooking something, the [ true, %2 ] -> [ false, %2 ] is pretty clearly wrong

Oh, Thanks, you are right. It seems there is a bug in alive2 , so I report it on https://github.com/AliveToolkit/alive2/issues/901

Allen updated this revision to Diff 516031.Apr 21 2023, 10:23 PM
Allen edited the summary of this revision. (Show Details)
Allen added a reviewer: aeubanks.

Fix the new added test WebrtcVideoStats, https://alive2.llvm.org/ce/z/EQtb_S

Allen added a comment.Apr 22 2023, 6:04 PM

hi @aeubanks, I fixed your case with alive2 proved the new generated IR.

would you help me check if there are any problems with the new version? Thanks

I reran the tests that failed before with this patch and now they pass. Feel free to reland, we'll see if any more issues are uncovered after relanding

Allen added a comment.Apr 24 2023, 6:09 PM

I reran the tests that failed before with this patch and now they pass. Feel free to reland, we'll see if any more issues are uncovered after relanding

Thanks very much

This revision was landed with ongoing or failed builds.Apr 25 2023, 4:41 AM
This revision was automatically updated to reflect the committed changes.

(unrelated, but is there any reason this pass is in the codegen pipeline instead of the optimization pipeline?)

nikic added a comment.Apr 27 2023, 1:13 PM

(unrelated, but is there any reason this pass is in the codegen pipeline instead of the optimization pipeline?)

The last attempt to move MergeICmps and ExpandMemCmp into the optimization pipeline ran into some issue (iirc related to sanitizers) and nobody looked into this in detail since. We would generally prefer to have them in the optimization pipeline, as the current backend position is too late to make full use of subsequent optimizations that may be enabled by them.

Allen added a comment.Apr 29 2023, 9:16 PM

Thanks for your report, and it seems the origin eq-chain also has the same issue, see detail https://alive2.llvm.org/ce/z/TDirEG
This is because the logic of memcmp , whose address of the comparison value is from small to large, so this problem needs to be solved independently first.

Allen added a comment.May 4 2023, 6:32 PM

hi @aeubanks, as the discussion on https://reviews.llvm.org/D149542, the miscompile (https://alive2.llvm.org/ce/z/Zr5dfP) doesn't need to be handled ?

hi @aeubanks, as the discussion on https://reviews.llvm.org/D149542, the miscompile (https://alive2.llvm.org/ce/z/Zr5dfP) doesn't need to be handled ?

hmm I swear the miscompile I was seeing wasn't related to poison/freeze, it was similar to the previous miscompile where it was just a wrong true/false. I'll take a look again

hi @aeubanks, as the discussion on https://reviews.llvm.org/D149542, the miscompile (https://alive2.llvm.org/ce/z/Zr5dfP) doesn't need to be handled ?

hmm I swear the miscompile I was seeing wasn't related to poison/freeze, it was similar to the previous miscompile where it was just a wrong true/false. I'll take a look again

I didn't see an issue reapplying this patch on the test case I was looking at so I've relanded it, sorry for the trouble

Allen added a comment.May 8 2023, 6:05 PM

Thank you for your careful verification!

@Allen this last patch is causing a miscompile again.

Here's a small artificial repro:

namespace {
struct Dt {
  Dt() = default;
};

class DtSet {
 public:
  DtSet() = default;
  ~DtSet() = default;

  class iterator {
   public:
    iterator(int a, int b, int idx)
        : a_(a), b_(b), idx_(idx) {}
    bool operator==(const iterator& other) const { return !(*this != other); }
    bool operator!=(const iterator& other) const;
   private:
    int a_, b_;
    int idx_;
  };

  iterator begin() const {
    if (0 == sz_) return end();
    int dt = 0;
    iterator iter(dt, dt+1, 0);
    return iter;
  }
  iterator end() const {
    return iterator(0, 0, sz_);
  }

 private:
  int sz_ = 0;

  DtSet(const DtSet&) = delete;
  DtSet& operator=(const DtSet&) = delete;
};

[[clang::noinline]]
bool DtSet::iterator::operator!=(const iterator& other) const {
  return idx_ != other.idx_ || b_ != other.b_ || a_ != other.a_;
}
}  // namespace

int main() {
  DtSet dt_set;
  if (dt_set.begin() == dt_set.end())
    return 0;
  else
    return 1;
}

Compilation command:

clang  -O3  -std=gnu++17  -pthread   \
   -o /tmp/out  \
   -x c++ /tmp/repro.cc

At this revision the program returns '1' (incorrect), at the previous revision the program returns '0' (as expected).

Could you please revert?

hi @aeubanks, as the discussion on https://reviews.llvm.org/D149542, the miscompile (https://alive2.llvm.org/ce/z/Zr5dfP) doesn't need to be handled ?

hmm I swear the miscompile I was seeing wasn't related to poison/freeze, it was similar to the previous miscompile where it was just a wrong true/false. I'll take a look again

I didn't see an issue reapplying this patch on the test case I was looking at so I've relanded it, sorry for the trouble

Oh that alive2 link is a miscompile even disregarding poison semantics. src is implementing a !=, but it ends up being a == with the memcmp after this pass runs (I got confused with the memcmp return value being 0 on equal instead of 1). Basically it should be
%3 = icmp eq i32 %memcmp, 0 instead of %3 = icmp ne i32 %memcmp, 0.

Allen added a comment.May 15 2023, 8:01 PM

Thanks for your analysis. The bug is brought by the following code in function mergeComparisons.
As the comparisons is reorded according the memory offset (https://reviews.llvm.org/D149542), so the origin last comparison BB may be not the Comparisons[Comparisons.size() - 1].BB

BasicBlock *const TailBB = Comparisons[Comparisons.size() - 1].BB;
Allen updated this revision to Diff 522426.May 15 2023, 9:16 PM

Add new function isLastLinkComparison to check whether the current comparisons is the last comparisons.

Allen added a comment.May 22 2023, 9:14 PM

@Allen this last patch is causing a miscompile again.

Here's a small artificial repro:

namespace {
struct Dt {
  Dt() = default;
};

At this revision the program returns '1' (incorrect), at the previous revision the program returns '0' (as expected).

Could you please revert?

I think the above case is fixed with last update, so I'll retry after rebase if nobody object it. sorry for the miscompile.

Allen updated this revision to Diff 525059.May 24 2023, 1:34 AM

rebase as precommit test D151091

This revision was landed with ongoing or failed builds.May 24 2023, 7:07 AM
bgraur added a comment.Jun 4 2023, 3:05 AM

@Allen the latest patch is again causing a similar miscompile as the previous one I reported.

We'll work on a reproducer tomorrow. Until then a revert would be greatly appreciated.

Allen added a comment.Jun 4 2023, 6:46 PM

@Allen the latest patch is again causing a similar miscompile as the previous one I reported.

We'll work on a reproducer tomorrow. Until then a revert would be greatly appreciated.

sorry again for the miscompile.
I don't mind to rock back this commit to reduce the interference with your work.

bgraur added a comment.Jun 5 2023, 3:07 AM

@Allen would you be so kind to revert the patch while we're working on the reproducer (might take some time)?

bgraur added a comment.EditedJun 6 2023, 4:57 AM

@Allen I got to a repro that requires a headers and two cc files.

The compilation command:

clang -O1  -std=gnu++17  -pthread \
  -x c++ \
  -o /tmp/out \
  impl.cc repro.cc

The reproducer will return 1 (incorrect) at this compiler and 0 (correct) at the version before.

Files are attached here:



Allen added a comment.Jun 6 2023, 6:14 PM

Thanks for your report , I think the miscompile is related to the splitting of ne-compare-chain.
In fact, it should not be splittted, I mistake this because the alive2 accept the transform, https://alive2.llvm.org/ce/z/iKMekA.
The above transform is invalid as I missing place the related target datalayout information.

target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64-unknown-linux-gnu"
class O {
  ...

  bool EqualsTo(const O &other) const {
    return first_ == other.first_ && type_ == other.type_
      && second_ == other.second_;
  }

 private:
  int type_ = 0;  // Discontinuous layout
  long first_;
  long second_;
};