• Check isSPIRVLogical rather than isSPIRV
• Improve diagnostics given that HLSL can generate both DXIL and SPIR-V
• Add tests for SPIR-V triples
• Fix missing REQUIRES: in dxil triple tests

Rebased to fix conflicts

Ping. Please take another look when you get a chance, thanks!

• Consolidate error messages
• Error messages should start with a lower case letter

LGTM. Have you thought about @aemerson's suggestion about a migration plan?

• Now with more const
• Added DiagnoseHLSLAttrStageMismatch and avoided using raw strings in diagnostics

Rebase

Address feedback and simplify valid shader stage test

Broke out a couple of semi-unrelated changes

I'm not entirely convinced by the motivation (wouldn't it be better to just fix the tests?), but this seems like a perfectly reasonable flag to have.

This looks like a good start. I think a fair amount of it will need to change as the HLSL support matures, but most of that should be fine to handle in tree. Probably needs an approval from somebody who's worked more in the SPIR-V backend than I have as well though.

In D153185#4590530, @Benjins wrote:

it was a bit unclear to me who would be best for reviewing C-API changes, looking at previous reviews in that area and CODE_OWNERS. I don't have any follow-up changes planned, but if there's a good person to mention on C-API changes that'd be good to know

In D153185#4441069, @Benjins wrote:

Apologies for bumping this: not sure who I should assign it to for review, since I couldn't find many recent C API diffs. Just want to make sure it doesn't get lost

Update release note to point at the reviews for conflict resolution instructions.

Rebase for Vis -> Visibility change in https://reviews.llvm.org/D157149, and a few fixes for the Default -> ClangOption change I missed on my last update.

Rebase on top of "Visibility" naming.

In D157151#4587522, @awarzynski wrote:

This might cause some disruption to downstream consumers of this API and Options.td. Hopefully, "update_options_td_flags.py" that you've included should minimise that. I suggest "advertising" it in the summary a bit more.

LGTM, great work, thank you!

When landing this, could you send a PSA to Discourse to make folks aware of what's coming? I am primarily concerned about our downstream users.

Rebased and finished the changes to use ClangOption everywhere instead of DefaultVis. I think this is good to go now, please take a look.

Rebase on top of "DefaultVis" naming

If the right answer is to skip functions that don't have line info we should probably do that in Context::print where we loop over file.functions or maybe just at the beginning of collectFunction - doing this on a per block basis inside of the function doesn't really make sense.

Update comment wording to be less ambiguous about flags vs visibility

I can't speak to which flags should be present in flang-new or not, but in principle this looks great. You'll need to update the patch to use the "ClangOption" spelling rather than "Default" of course, as discussed in https://reviews.llvm.org/D157151

In D157151#4583904, @awarzynski wrote:

This can be tweaked in getOptionVisibilityMask (extracted from this patch):

llvm::opt::Visibility
Driver::getOptionVisibilityMask(bool UseDriverMode) const {
  if (!UseDriverMode)
    return llvm::opt::Visibility(llvm::opt::Default);
  if (IsCLMode())
    return llvm::opt::Visibility(options::CLOption);
  if (IsDXCMode())
    return llvm::opt::Visibility(options::DXCOption);
  if (IsFlangMode()) {
    // vvvvvvvvvvvvvvv HERE vvvvvvvvvvvvvvvvvvv
    // TODO: Does flang really want *all* of the clang driver options?
    // We probably need to annotate more specifically.
    return llvm::opt::Visibility(llvm::opt::Default | options::FlangOption);
  }
  return llvm::opt::Visibility(llvm::opt::Default);
}

Now, prior to this change I couldn't find a way to disable unsupported Clang options in Flang. With this patch,

• all Clang options gain a visibility flag (atm that's Default),
• that flag can be used to disable options unsupported in Flang.

For this to work, all options supported by Flang need to have their visibility flags set accordingly. That's the goal, but I can see that we have missed quite a few options over the time (*). Updated here: https://reviews.llvm.org/D157837.

Update to use "ClangOption" instead of "Default" (See https://reviews.llvm.org/D157150 for the introduction of the name), and address documentation comments.

Use "ClangOption" rather than "Default" in Options.td, as per the conversation in https://reviews.llvm.org/D157151

In D157151#4582109, @awarzynski wrote:

I think that it would be good to replace Default with e.g.

• Clang, or
• ClangDriver, or
• ClangCompilerDriver.

Or, at least, to make the meaning of Default much clearer (through e.g. comments). In general, I feel that Default is skewed towards this notion that clang (the compiler driver tool) is the main client of clangDriver. That used to be the case, but with Flang's driver also implemented in terms of clangDriver , that's no longer the case. Also, the long term goal is to extract the driver library out of Clang. One day :)

Update FlangDriver.md

In D157151#4577790, @awarzynski wrote:

Hey @bogner , I've only skimmed through so far and it's looking great! That Include/Exclude API was not fun to use. What you are proposing here takes Options.td to a much a better place - this is a much needed and long overdue refactor.

There's quite a bit of churn here, so I will need a few days to scan. In the meantime, could you update flang/docs/FlangDriver.md? And in general, please note that this updates (primarily) clangDriver logic, which is used by both by Clang and Flang. In particular, Options.td is shared. I think that it's worth highlighting that this change benefits both sub-projects.