This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add support for auxiliary triple specification
AbandonedPublic

Authored by oToToT on Feb 19 2021, 9:39 PM.

Details

Summary

I was working on CUDA support for clangd, and I've found that clangd is missing support for auxiliary triple specification. Thus, if we try making clangd work with CUDA or other languages that requires auxiliary triple, it will produce some wrong results.

After some investigation of what clang actually did, I borrowed some code from clang/lib/Frontend/CompilerInstance.cpp to make clangd work as well.

I don't know whether this fix is reasonable or not, but it just fix something looks like a problem in clangd.

Diff Detail

Event Timeline

oToToT created this revision.Feb 19 2021, 9:39 PM
oToToT requested review of this revision.Feb 19 2021, 9:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 9:39 PM

Please upload diffs with full context. This can be done by passing -U999999 to diff or by using arcanist to upload your patches.

oToToT updated this revision to Diff 325186.Feb 20 2021, 4:03 AM

re-upload diff with full context, sorry for that.

oToToT added inline comments.Feb 20 2021, 4:09 AM
clang/lib/Frontend/PrecompiledPreamble.cpp
311

Though this file is in clang, clangd use this function to prepare preamble in Preamble.cpp, and I think it is OK to add support for auxiliary triple here.

jdoerfert resigned from this revision.Feb 21 2021, 8:59 PM
oToToT edited reviewers, added: jlebar, rsmith; removed: jdoerfert.Feb 22 2021, 5:08 AM

Thanks for working this out!

Clearly there are some layering concerns here - clangd is "just" a clang tool that parses some code, and this is some pretty hairy clang internal logic we're copying here.
Practical concerns:

  • it it could very easily skew from clang (e.g. will Sycl + CUDA + OpenMP always be the right list?!)
  • there are other tools (callers of setTarget) that also need this logic
  • this isn't even all the target logic we're missing, there's also fpenv stuff in CompilerInstance::ExecuteAction

Moreover this is entirely propagating settings from CompilerInvocation (spec) to CompilerInstance (state), which naturally belongs in ComplilerInstance.

I'd suggest this plan:

  1. extract the target-related logic in CompilerInstance::ExecuteAction to a method createTarget(). This is NFC
  2. update clangd + PrecompiledPreamble to call createTarget() instead of setTarget() +getTarget()->adjust() etc. This fixes this bug and others in clangd.
  3. (optional, for you or me or others) update other callers of setTarget() in a similar way, which simplifies them and fixes bugs there too.

WDYT?

oToToT abandoned this revision.Feb 23 2021, 8:29 AM

Thanks sammccall for reviewing this and for the great idea of the whole plan.

I will work on the plan (maybe without the third one) to fix the problem described here.

Sorry for a poor submission here again.

No apology necessary!

I think it's worth working out where best to put the fix, but the fix itself is the hard part from my perspective (not knowing much about cuda and other dialects). So this is much appreciated.

Please let me know if you hit any problems with the suggested refactoring, I don't want this to get lost.