This is an archive of the discontinued LLVM Phabricator instance.

Set MAttrs in LTO mode
ClosedPublic

Authored by yinma on Oct 19 2018, 11:58 AM.

Diff Detail

Event Timeline

yinma created this revision.Oct 19 2018, 11:58 AM
yinma added a reviewer: ruiu.Oct 19 2018, 12:01 PM
pcc requested changes to this revision.Oct 19 2018, 12:09 PM
pcc added a subscriber: pcc.

The MAttrs part is fine but we don't want CGOptLevel to be set based on the LTO opt level but rather on the opt level passed at compile time. See mailing list thread "[RFC] Adding function attributes to represent codegen optimization level" for more information.

This revision now requires changes to proceed.Oct 19 2018, 12:09 PM
yinma updated this revision to Diff 170235.Oct 19 2018, 12:16 PM
yinma retitled this revision from Set CGOptLevel and MAttrs in LTO mode to Set MAttrs in LTO mode.
yinma edited the summary of this revision. (Show Details)

I see you point. I have removed CGOptLevel portion

MaskRay added inline comments.Oct 23 2018, 11:22 AM
Common/TargetOptionsCommandFlags.cpp
36

return std::vector<std::string>(MAttrs.begin(), MAttrs.end());

ruiu added inline comments.Oct 23 2018, 11:26 AM
Common/TargetOptionsCommandFlags.cpp
38

Where is this MAttrs defined?

yinma added inline comments.Oct 23 2018, 11:44 AM
Common/TargetOptionsCommandFlags.cpp
36

OK

38

Those can be set by commend line options

ruiu added inline comments.Oct 23 2018, 12:00 PM
Common/TargetOptionsCommandFlags.cpp
38

But I can't find that code in this patch. Is that is another patch?

yinma added inline comments.Oct 23 2018, 12:18 PM
Common/TargetOptionsCommandFlags.cpp
38

I see what you are asking for. it is defined in LLVM.

include/llvm/CodeGen/CommandFlags.inc:40: MAttrs("mattr", cl::CommaSeparated,

ruiu added inline comments.Oct 23 2018, 1:07 PM
Common/TargetOptionsCommandFlags.cpp
36

Please format carefully -- you need a space between () and the following {. I'd recommend clang-format-diff if you are not using it already.

38

Oh, I missed that. So MAttrs is not even in any namespace? Not sure if that is a good practice. Anyway, please add a comment saying that "MAttrs is defined in llvm/CodeGen/CommandFlags.inc."

I believe you can simply return MAttrs from this function.

ruiu added inline comments.Oct 23 2018, 1:09 PM
Common/TargetOptionsCommandFlags.cpp
38

Or, maybe the easiest way of writing this function is just this:

std::vector<std::string> lld::GetMAttrs() { return ::MAttrs; }
ruiu added inline comments.Oct 23 2018, 1:20 PM
ELF/LTO.cpp
92–93

I don't think we need this comment as we don't have one for GetCPUStr() and others. For lld, they are just opaque arguments passed from users to the LTO backend.

yinma updated this revision to Diff 171379.Oct 26 2018, 5:27 PM
MaskRay added inline comments.Oct 26 2018, 5:36 PM
COFF/LTO.cpp
63

Delete the empty line?

yinma updated this revision to Diff 172185.Nov 1 2018, 11:12 AM
MaskRay accepted this revision.Nov 1 2018, 11:27 AM
pcc accepted this revision.Nov 1 2018, 1:01 PM

LGTM

This revision is now accepted and ready to land.Nov 1 2018, 1:01 PM

I'll commit on behalf of @yinma as he contacted me to do that.

This revision was automatically updated to reflect the committed changes.