This is an archive of the discontinued LLVM Phabricator instance.

Expose ScalarizerPass options to C++ (not just commandline)
ClosedPublic

Authored by Benoit on Mar 7 2022, 8:45 PM.

Details

Summary

Context: I needed this for https://github.com/google/iree/pull/8474 . I found that TSan instrumentation expects vector sizes to be <= 16, and in my project (IREE) we have tests with higher vector sizes. That left some test functions uninstrumented, resulting in crashes as instrumented code called into them.

Diff Detail

Event Timeline

Benoit created this revision.Mar 7 2022, 8:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 8:45 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Benoit requested review of this revision.Mar 7 2022, 8:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 8:45 PM
Benoit added a comment.Mar 7 2022, 9:01 PM

Context: I needed this for https://github.com/google/iree/pull/8474 . I found that TSan instrumentation expects vector sizes to be <= 16, and in my project (IREE) we have tests with higher vector sizes. That left some tests function uninstrumented, resulting in crashes as instrumented called into them.

Benoit retitled this revision from Expose ScalarizerPass options to C++ (not just commandline) to (WIP - DO NOT REVIEW YET) Expose ScalarizerPass options to C++ (not just commandline).Mar 7 2022, 9:01 PM
Benoit updated this revision to Diff 413990.Mar 8 2022, 6:35 PM

clang-format

Benoit retitled this revision from (WIP - DO NOT REVIEW YET) Expose ScalarizerPass options to C++ (not just commandline) to Expose ScalarizerPass options to C++ (not just commandline).Mar 8 2022, 6:43 PM
Benoit added reviewers: vettoreldaniele, bjope.
Benoit edited the summary of this revision. (Show Details)Mar 8 2022, 6:45 PM
bjope added inline comments.Mar 9 2022, 12:05 AM
llvm/lib/Transforms/Scalar/Scalarizer.cpp
194

With the current patch you won't be able to override any pass options setup by the pass manager etc.
I would expect that the cmd line value has higher precedence.

Benoit updated this revision to Diff 414098.Mar 9 2022, 7:32 AM

give the cl opts precedence. per review comment

Benoit added inline comments.Mar 9 2022, 7:33 AM
llvm/lib/Transforms/Scalar/Scalarizer.cpp
194

Thanks, that makes sense. Code updated.

Benoit edited the summary of this revision. (Show Details)Mar 9 2022, 1:37 PM
Benoit marked an inline comment as done.Mar 9 2022, 6:58 PM
bjope accepted this revision.Mar 10 2022, 1:22 AM

Just a few nits. But otherwise this LGTM.

llvm/include/llvm/Transforms/Scalar/Scalarizer.h
38

nit: This pass use CamelCase for variable names.

43

nit: This pass use CamelCase for variable names.

46

nit: This pass use CamelCase for variable names.

This revision is now accepted and ready to land.Mar 10 2022, 1:22 AM
Benoit updated this revision to Diff 414384.Mar 10 2022, 8:17 AM

review comments

Benoit marked 3 inline comments as done.Mar 10 2022, 8:17 AM

I don't have write access. Can someone please push this for me?

This revision was landed with ongoing or failed builds.Mar 14 2022, 4:01 AM
This revision was automatically updated to reflect the committed changes.
bjope added a comment.Mar 14 2022, 4:01 AM

I don't have write access. Can someone please push this for me?

I've now landed this for you.