This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Relax conditions for address space cast in builtin args
ClosedPublic

Authored by gandhi21299 on Oct 13 2021, 8:40 AM.

Details

Diff Detail

Event Timeline

gandhi21299 requested review of this revision.Oct 13 2021, 8:40 AM
gandhi21299 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 8:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

removed irrelevant lines in the test

we need a codegen test for the case

clang/lib/Sema/SemaExpr.cpp
6550

we still need to keep this condition so it should be:

if ((ArgAS != LangAS::Default &&
        getASTContext().getTargetAddressSpace(ArgAS) !=
        getASTContext().getTargetAddressSpace(ParamAS))
   || ParamAS == LangAS::Default)
gandhi21299 marked an inline comment as done.

adding codegen test

tra added a subscriber: tra.Oct 13 2021, 10:50 AM

I think for a Sema change there should be a Sema test, too.

Passed internal CI, still working on a Sema test.

adding sema test

installed clang-format, refreshing patch

removed unused diagnostic sema note

tra added inline comments.Oct 15 2021, 10:41 AM
clang/lib/Sema/SemaExpr.cpp
6548–6553

I'd rephrase it in terms of add ASC if target AS is different.

Prevent assumes that we've already decided to add an ASC, while it is not the case. All we know is that we've got a pointer argument and we are figuring out whether ASC is needed.

clang/test/SemaCUDA/builtins-unsafe-atomics-gfx90a.cu
4

You do not want to produce any output. I think -fsyntax-only -verify along with // expected-no-diagnostics should do the trick.

9

Not needed w/o FileCheck.

gandhi21299 marked 3 inline comments as done.

corrected sema test, as requested

yaxunl accepted this revision.Oct 15 2021, 12:42 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Oct 15 2021, 12:42 PM

Thanks for the review!

tra accepted this revision.Oct 15 2021, 12:54 PM

LGTM in general, modulo a readability suggestion.

clang/lib/Sema/SemaExpr.cpp
6548–6553

Hmm. I guess that does not quite match what we do here.
Oringal code would add ASC for generic->specific only.
The test cases you've addedd are for specific->specific AS, AFAICT, and that makes me wonder if I understand what's going on here.

So the test case currently does fail, because we end up w/o implicit ASC and end up trying to pass wrong types of pointers. https://godbolt.org/z/MaWMbGb5z

Now we actually want to allow implicit ASC for the pointer types that are in the *same* target AS, so my comment suggestion was actually wrong.

This confusion also suggests that the condition is hard to understand. Perhaps we can rewrite it.

E.g.

bool NeedImplicitASC = 
      ParamAS != LangAS::Default &&   // Pointers params in generic AS don't need special handling.
      ( ArgAs == LangAS::Default  ||         // We do allow implicit conversion from generic AS 
        // or from specific AS which has target AS matching that of Param.
        getASTContext().getTargetAddressSpace(ArgAS) == getASTContext().getTargetAddressSpace(ParamAS));
if (!NeedImplicitASC)
   continue;

@tra I see, we sure can rewrite that segment for readability.

gandhi21299 reopened this revision.Oct 15 2021, 1:44 PM
This revision is now accepted and ready to land.Oct 15 2021, 1:44 PM
tra added a comment.Oct 15 2021, 1:47 PM

@tra I see, we sure can rewrite that segment for readability.

It could've been done with a follow-up clean-up commit. revert/reland works, too. Thank you for following up on the comment, either way.

cleaned up code for readability

This revision was landed with ongoing or failed builds.Oct 15 2021, 2:36 PM
This revision was automatically updated to reflect the committed changes.