This is an archive of the discontinued LLVM Phabricator instance.

[msan] Instrument x86.pclmulqdq* intrinsics.
ClosedPublic

Authored by eugenis on Jan 24 2020, 11:12 AM.

Details

Summary

These instructions ignore parts of the input vectors which makes the
default MSan handling too strict and causes false positive reports.

Diff Detail

Event Timeline

eugenis created this revision.Jan 24 2020, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 11:12 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Unit tests: pass. 62183 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Unit tests: pass. 62183 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

RKSimon added inline comments.
llvm/test/Instrumentation/MemorySanitizer/clmul.ll
29

worth adding a llvm.x86.pclmulqdq.256 test case?

@thakis Please can you confirm this fixes the original bug in your codebase (not just the reduced case)?

craig.topper added inline comments.Jan 24 2020, 12:37 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
3003

Maybe the Imm arg should just be something like "bool OddElements".

3027

This should be a cast, not a dyn_cast.

@thakis Please can you confirm this fixes the original bug in your codebase (not just the reduced case)?

Not easily. But if this lands, we'll know within a few hours if it helps, we have a bot that does this.

I did check that it helps with the reduced repro (it does).

eugenis updated this revision to Diff 240277.Jan 24 2020, 12:52 PM

address comments

eugenis marked 3 inline comments as done.Jan 24 2020, 12:52 PM

Unit tests: pass. 62183 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

thakis accepted this revision.Jan 24 2020, 1:17 PM
This revision is now accepted and ready to land.Jan 24 2020, 1:17 PM
This revision was automatically updated to reflect the committed changes.

Not easily. But if this lands, we'll know within a few hours if it helps, we have a bot that does this.

Just to follow up here, the bot's happy. So this fixes all the issues we see.