Page MenuHomePhabricator

[InstSimpify][ValueTracking] Use new FP matchers
Needs ReviewPublic

Authored by foad on Oct 23 2020, 2:00 AM.

Details

Diff Detail

Unit TestsFailed

TimeTest
380 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

foad created this revision.Oct 23 2020, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 2:00 AM
foad requested review of this revision.Oct 23 2020, 2:00 AM

I split this out from D89038 because of @lebedev.ri's comment:

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.
Because while the matchers themselves LG, i'm not at all confident with users.
When doing latter, please ensure that each separate change has appropriate undef test coverage, and ideally check that alive2 is happy about those transforms.

I haven't looked at adding the extra test coverage yet.

I split this out from D89038 because of @lebedev.ri's comment:

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.
Because while the matchers themselves LG, i'm not at all confident with users.
When doing latter, please ensure that each separate change has appropriate undef test coverage, and ideally check that alive2 is happy about those transforms.

I haven't looked at adding the extra test coverage yet.

Sorry, I'm overloaded with reviews, but I agree with @lebedev.ri that it was good to split this up. I'd go even further and split this patch up too and add tests for all known callers of the ValueTracking APIs. We've seen several cases of subtle bugs with partial-undef vectors, so we need more tests and ideally confirm correctness with Alive2.

I split this out from D89038 because of @lebedev.ri's comment:

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.
Because while the matchers themselves LG, i'm not at all confident with users.
When doing latter, please ensure that each separate change has appropriate undef test coverage, and ideally check that alive2 is happy about those transforms.

I haven't looked at adding the extra test coverage yet.

Sorry, I'm overloaded with reviews, but I agree with @lebedev.ri that it was good to split this up. I'd go even further and split this patch up too and add tests for all known callers of the ValueTracking APIs. We've seen several cases of subtle bugs with partial-undef vectors, so we need more tests and ideally confirm correctness with Alive2.

+1