Page MenuHomePhabricator

[AMDGPU][OpenMP] Fix duplicate copies of arguments in commands
AbandonedPublic

Authored by pdhaliwal on Jun 2 2020, 5:28 AM.

Details

Summary

When offloading kind is OFK_OpenMP, the host toolchain (Generic_GCC)
returns DerivedArgList which copies input arguments along with few
other arguments. HIPToolchain again copies input arguments to the
same DerivedArgList.

Diff Detail

Event Timeline

pdhaliwal created this revision.Jun 2 2020, 5:28 AM
Herald added a project: Restricted Project. · View Herald Transcript
yaxunl added a comment.Jun 2 2020, 5:57 AM

Can we have a lit test? Thanks.

pdhaliwal updated this revision to Diff 267870.Jun 2 2020, 6:50 AM

Added lit test case

arsenm added a subscriber: arsenm.Jun 2 2020, 7:34 AM
arsenm added inline comments.
clang/lib/Driver/ToolChains/HIP.cpp
389–392

Needs a comment? I don't understand why openmp is any different here

pdhaliwal added inline comments.Jun 2 2020, 8:25 AM
clang/lib/Driver/ToolChains/HIP.cpp
389–392

HostTC.TranslateArgs (HostTC = Generic_GCC, Line#383) returns DAL which contains Args when offloadkind is OFK_OpenMP (for all other cases, it returns nullptr). Thus, Line#{390,391} is just duplicating the arguments which are propagating to the opt, llc, etc. commands.
Ref: https://clang.llvm.org/doxygen/Gnu_8cpp_source.html#l02966

sameerds added inline comments.Jun 3 2020, 7:37 PM
clang/lib/Driver/ToolChains/HIP.cpp
389–392

I think @arsenm was asking for a comment in the code itself.

Also, I am not sufficiently familiar with the design here, but why is the HIP driver checking for OpenMP offloading? Is the offloaded region treated as HIP code? It seems a bit strange that we are mixing two different things in the same driver.

pdhaliwal abandoned this revision.Jun 4 2020, 6:01 PM
pdhaliwal marked an inline comment as done.
pdhaliwal added inline comments.
clang/lib/Driver/ToolChains/HIP.cpp
389–392

@sameerds you are correct, openmp should not be mixing things in HIP driver. So, I am dropping this patch before it creates more confusion.