This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Diagnose thinLTO usage in clang on AIX.
ClosedPublic

Authored by w2yehia on Apr 12 2021, 4:55 PM.

Diff Detail

Event Timeline

w2yehia created this revision.Apr 12 2021, 4:55 PM
w2yehia requested review of this revision.Apr 12 2021, 4:55 PM
Xiangling_L added inline comments.Apr 13 2021, 8:06 AM
clang/lib/Driver/Driver.cpp
4006

I would suggest a better way to specify the target is to use err_drv_unsupported_opt_for_target like above rather than hardcoding.

w2yehia added inline comments.Apr 13 2021, 11:14 AM
clang/lib/Driver/Driver.cpp
4006

using err_drv_unsupported_opt_for_target requires an option spelling, which implies we need an option present. Here I wanted to diagnose that thinLTO is not supported on AIX regardless of how we reached the decision that LTOMode is LTOK_Thin (for example, maybe -flto will start implying LTOK_Thin)

So we have two options:

  1. diagnose thinLTO in general: I can change the diagnostics to the following (which is kinda ugly but couldn't find a shorter way without introducing a new diagnostics message):
Diag(diag::err_drv_clang_unsupported)
    << ("thinLTO on target '" + RawTriple.str() + "'").c_str();
  1. diagnose -flto=thin option only:
if (Arg *A = C.getArgs().getLastArg(options::OPT_flto_EQ))
  if (!strcmp(A->getValue(), "thin"))
     Diag(diag::err_drv_unsupported_opt_for_target)
         << (A->getSpelling() + std::string("thin")).c_str() 
         << RawTriple.str();

Please, let me know which you prefer and whether there's another way to do this.
Thanks.

Xiangling_L added inline comments.Apr 13 2021, 2:23 PM
clang/lib/Driver/Driver.cpp
4006

Thanks for your explanation and all possibilities provided. I think it makes sense to keep what you already have in this patch.
Just one more thing, based on this patch, I think a confusing part is that you put diagnose into aix-err-options.c file but the diagnose is not about which option AIX doesn't support. And as you mentioned, you'd like to diagnose that thinLTO is not supported on AIX regardless of how we reached the decision. Should we create a new test file for it?

w2yehia updated this revision to Diff 337521.Apr 14 2021, 12:35 PM

moved the testcase to its own file as per reviewer's suggestion.

Xiangling_L accepted this revision.Apr 15 2021, 6:49 AM

LGTM. thanks

This revision is now accepted and ready to land.Apr 15 2021, 6:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2021, 9:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript