[PATCH] D79796: Sketch support for generating CC1 command line from CompilerInvocation

[Daniel Grumberg via Phabricator via cfe-commits]

dang marked 2 inline comments as done.
dang added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3845
+      IS_POSITIVE != DEFAULT_VALUE && this->KEYPATH != DEFAULT_VALUE)          \
+    Args.push_back(StringAllocator(Twine(PREFIX_TYPE[0]) + NAME));
+#include "clang/Driver/Options.inc"
----------------
dexonsmith wrote:
> Bigcheese wrote:
> > It's a little sad that we need to allocation every string just because of the `-`. We definitely need to be able to allocate strings for options with data, but it would be good if we could just have the strings with `-` prefixed in the option table if that's reasonable to do.
> I want to highlight @Bigcheese's comment again so it doesn't get lost. I think a reasonable name would be `SPELLING`.
Yes I haven't forgotten just have not gotten around to it yet. I just want to do some form of suffix merging with the string the the OptTable receives


================
Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:351-354
+    OS << "#define " << MacroName << "(V, D) .Case(V, D)\n";
+    emitValueTable(OS, OptionName, R.getValueAsString("Values"),
+                   NormalizedValuesScope, NormalizedValues);
+    OS << "#undef " << MacroName << "\n";
----------------
dexonsmith wrote:
> It seems unnecessary to generate macros here; you can just generate the code directly...
> 
> But looking more closely, I wonder if we really need this generated code at all.
> 
> Instead, can we create a table like this in Options.inc?
> ```
> struct EnumValue {
>   const char *Name;
>   unsigned Value;
> };
> 
> struct EnumValueTable {
>   const EnumValue *Table;
>   unsigned Size;
> };
> 
> static const EnumValue RelocationModelTable[] = {
>   {"static", static_cast<unsigned>(llvm::Reloc::Static)},
>   {"pic", static_cast<unsigned>(llvm::Reloc::_PIC)},
>   // ...
> };
> 
> static const EnumValue SomeOtherTable[] = {
>   {"name", static_cast<unsigned>(clang::SomeOtherEnumValue)},
>   // ...
> };
> 
> static const EnumValueTable *EnumValueTables[] = {
>   {&RelocationModelTable, sizeof(RelocationModelTable) / sizeof(EnumValue)},
>   {&SomeOtherTable, sizeof(SomeOtherTable) / sizeof(EnumValue)},
> };
> static const unsigned EnumValueTablesSize =
>     sizeof(EnumValueTables) / sizeof(EnumValueTable);
> ```
> 
> We can then have this code directly in Options.cpp:
> ```
> static llvm::Optional<unsigned> extractSimpleEnum(
>     llvm::opt::OptSpecifier Opt, int TableIndex,
>     const ArgList &ArgList, DiagnosticsEngine &Diags) {
>   assert(TableIndex >= 0);
>   assert(TableIndex < EnumValueTablesSize);
>   const EnumValueTable &Table = *EnumValueTables[TableIndex];
> 
>   auto Arg = Args.getLastArg(Opt);
>   if (!Arg)
>     return None;
> 
>   StringRef ArgValue = Arg->getValue();
>   for (int I = 0, E = Table.Size; I != E; ++I)
>     if (ArgValue == Table.Table[I].Name)
>       return Table.Table[I].Value;
> 
>   Diags.Report(diag::err_drv_invalid_value)
>       << Arg->getAsString(ArgList) << ArgValue;
>   return None;
> }
> ```
> and change the normalizer contract to:
> - return an `Optional`,
> - take an `llvm::opt::OptSpecifier`,
> - take an index into a table (set to `-1` if there's no relevant table), and
> - be responsible for the boilerplate call to `getLastArg`.
> Simple enums would have `NORMALIZER` hardcoded to `extractSimpleEnum`. The handler code that calls it needs new arguments `TABLE_INDEX` and `TYPE` but also looks simpler this way:
> ```
> if (auto MaybeValue = NORMALIZER(Opt##ID, TABLE_INDEX, Args, Diags)) \
>   this->KEYPATH = static_cast<TYPE>(*MaybeValue);                    \
> else                                                                 \
>   this->KEYPATH = DEFAULT_VALUE;
> ```
> 
> We could similarly have a single `serializeSimpleEnum` function in Options.cpp for a hardcoded `DENORMALIZER` for simple enums:
> ```
> static const char *serializeSimpleEnum(int TableIndex, unsigned Value) {
>   assert(TableIndex >= 0);
>   assert(TableIndex < EnumValueTablesSize);
>   const EnumValueTable &Table = *EnumValueTables[TableIndex];
>   for (int I = 0, E = Table.Size; I != E; ++I)
>     if (Value == Table.Table[I].Value)
>       return Table.Table[I].Name;
> 
>   llvm::report_fatal_error("good error message");
> }
> ```
I originally wanted to generate the normalizers without the table to allow and arbitrary code fragment to be used as the normalized value (one that is potentially not a constant evaluated expression) and just regenerating the macros was the path of least resistance at the time. Having had a closer look at the existing cases of enum based options there doesn't seem to be a need for this functionality so I went ahead and did this. Although I haven't split the backends yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79796/new/

https://reviews.llvm.org/D79796