This is an archive of the discontinued LLVM Phabricator instance.

[InstSimpify][ValueTracking] Use new FP matchers
Changes PlannedPublic

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

Details

Diff Detail

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

RKSimon resigned from this revision.Mar 30 2021, 2:37 AM
lebedev.ri resigned from this revision.Jan 12 2023, 5:21 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:21 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
foad planned changes to this revision.Jan 13 2023, 2:11 AM