Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 60,080 ms | x64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
| |
| 1,960 ms | x64 debian > libFuzzer.libFuzzer::set_cover_merge.test Script:
--
: 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/FullCoverageSetTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/set_cover_merge.test.tmp-FullCoverageSetTest
|
Event Timeline
ping :) @nikic can you please review this revision to make sure it is what you want in https://github.com/llvm/llvm-project/issues/54561#issuecomment-1079460876 ?
(I'm new to around this, so those comments are just my opinions.)
| llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp | ||
|---|---|---|
| 1307 | IMHO, duplicating SuccForValue map for each casts seems a little redundant. I like collecting incoming phi values in the front and checking correspondences to unify those maps. Then we can also early return if the values don't correspond. | |
| 1379–1381 | Inverting can't be determined on these lines. If you do so, these comments look obsolete, and better to be modified | |
| 1385 | IMHO, this checking, which is done when creating maps, seems a bit redundant, it's better to hold conditions in some way. | |
| llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp | ||
|---|---|---|
| 1307 | Thanks for the feedback!
Can you elaborate your solution? Here are my worries: Here we are going to find a operation that maps branch condition in to corresponding phi values. Not an operation that maps phi values back to branch condition. I think I have to left "sext"/"zext"ed values here because we must know the result of the branch condition after "s/zext". That's different from truncating from PHI values. i.e. s/zext information is not recoverable. Truncating phi values to branch condition does not imply we can z/sext the branch condition back to "phi values". | |
| 1379–1381 | Nice catch! I'd like to fixup this after all "functional change" reviews. | |
| llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp | ||
|---|---|---|
| 1307 | Sorry for my vague comments. The idea was unifying later inverting checking and these casting checking. Finding SExt or ZExt is similar to decide an inversion
I just thought it'll be neat to reverse the roll between Succ/Cond and Pred/Phi. Then we don't need to hold candidate values. But I feel this might be too detailed and optional, this requires the dominant condition reversed. (I just tried to write actually, but ugly wip https://github.com/llvm/llvm-project/compare/main...khei4:llvm-project:wip/khei4-sext | |