Page MenuHomePhabricator

[tooling] Teach Tooling to understand compilation with offloading.
ClosedPublic

Authored by hliao on Tue, Oct 8, 12:36 PM.

Diff Detail

Event Timeline

hliao created this revision.Tue, Oct 8, 12:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 8, 12:36 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tra added a reviewer: klimek.Tue, Oct 8, 1:51 PM

Added Manuel as someone familiar with tooling.

clang/lib/Tooling/Tooling.cpp
126

Is this still the right job for us to pick? I think we want this to be the host compilation.

As things are right now my guess is that we're probably picking the first device-side compilation.

hliao marked an inline comment as done.Wed, Oct 9, 6:21 AM
hliao added inline comments.
clang/lib/Tooling/Tooling.cpp
126

yes, OffloadAction is appended to the original host action. The corresponding first job is the host compilation. The frontend has similar handling in lib/Frontend/CreateInvocationFromCommandLine.cpp

hliao added a comment.Wed, Oct 9, 11:51 AM

PING for review

tra added inline comments.Wed, Oct 9, 11:59 AM
clang/lib/Tooling/Tooling.cpp
126

Can we add an assert or diagnostics for that? I think the check below would not always be able to tell if it unintentionally picked a wrong kind of job.

hliao marked an inline comment as done.Wed, Oct 9, 12:09 PM
hliao added inline comments.
clang/lib/Tooling/Tooling.cpp
126

It seems to me that the cast<> and check against "clang" @ L118 already did what I asked.

tra added inline comments.Wed, Oct 9, 1:47 PM
clang/lib/Tooling/Tooling.cpp
126

Device-side compilation would also use clang, so the check for clang is insufficient for ensuring that it's a *host* compilation.

Furthermore, the order of jobs *will* change depending on whether -fsyntax-only is in effect.

@klimek -- Manuel, Is tooling allowed to create an invocation w/o -fsyntax-only? AFAICT, runToolOnCodeWithArgs always adds it, but one could technically create an Invocation without it.

klimek added inline comments.Thu, Oct 10, 12:09 AM
clang/lib/Tooling/Tooling.cpp
126

Generally, tooling is allowed to do it, but it's not super well supported or tested.

hliao updated this revision to Diff 224443.Thu, Oct 10, 12:18 PM

add more assertions on offload compilation.

hliao marked an inline comment as done.Thu, Oct 10, 12:20 PM
hliao added inline comments.
clang/lib/Tooling/Tooling.cpp
126

I added more offload compilation based on actions, which is more reliable that jobs. Is that sufficient?

tra added inline comments.Thu, Oct 10, 1:29 PM
clang/lib/Tooling/Tooling.cpp
102

Why 2? Will it not be different if user targeted multiple GPUs?

103

I'd add a comment describing that we expect the first jub to be a (host-side)compilation.
It would not be obvious to someone w/o familiarity of cuda/hip compilation.

hliao updated this revision to Diff 224476.Thu, Oct 10, 2:16 PM
hliao marked an inline comment as done.

revice comment.

hliao marked an inline comment as done.Thu, Oct 10, 2:17 PM
hliao added inline comments.
clang/lib/Tooling/Tooling.cpp
102

that's the number of top-level actions. no matter how many GPUs are specified, that offload action is the top-level action masters all of them.

tra accepted this revision.Thu, Oct 10, 2:29 PM
This revision is now accepted and ready to land.Thu, Oct 10, 2:29 PM
This revision was automatically updated to reflect the committed changes.
hliao added a comment.Thu, Oct 10, 4:44 PM

This makes tests assert on Mac: http://45.33.8.238/mac/1415/step_6.txt

I have no access to MacOS but try to fix that in r374478. Maybe that's a good excuse for a MacBook, ;)

hliao added a comment.Thu, Oct 10, 5:08 PM

This makes tests assert on Mac: http://45.33.8.238/mac/1415/step_6.txt

please let me know whether that works for you. thanks.