This is an archive of the discontinued LLVM Phabricator instance.

[msan] Handle funnel shifts
ClosedPublic

Authored by vitalybuka on Jul 2 2021, 7:23 PM.

Diff Detail

Event Timeline

vitalybuka created this revision.Jul 2 2021, 7:23 PM
vitalybuka requested review of this revision.Jul 2 2021, 7:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2021, 7:23 PM
vitalybuka retitled this revision from [msan] Handle funel shifts to [msan] Handle funnel shifts.Jul 2 2021, 7:24 PM
vitalybuka added a reviewer: eugenis.

Instrumentation change LGTM but the test looks way too complicated. I'd edit it down to only check the lines that matter, split to have one intrinsic call per function (that should simplify matching quite a bit) and drop some of the vector-size variants.

I'm also not a fan of adding a test for the broken state before the fix, but up to you. I can see how it is useful during development, but not sure what's the point of having it in git history.

Instrumentation change LGTM but the test looks way too complicated. I'd edit it down to only check the lines that matter, split to have one intrinsic call per function (that should simplify matching quite a bit) and drop some of the vector-size variants.

It's auto-generated test, manual cleunup will make it impossible auto-update

I'm also not a fan of adding a test for the broken state before the fix, but up to you. I can see how it is useful during development, but not sure what's the point of having it in git history.

To see the difference.

eugenis accepted this revision.Jul 7 2021, 11:24 PM

OK then, LGTM.

Btw, we are lucky that setOriginForNaryOp is right-biased. If both the shift size and one of the other arguments are undefined, we would want to blame the shift size, because the undefined bits from the first 2 arguments may get shifted away. To be precise, we would still want to mask out the extra bits in the shift size argument before using it in the origin computation, but that's a mostly theoretical concern.

This revision is now accepted and ready to land.Jul 7 2021, 11:24 PM
This revision was automatically updated to reflect the committed changes.