This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Cleanup support for `this` as an l-value
ClosedPublic

Authored by beanz on Aug 30 2023, 7:11 PM.

Details

Summary

The goal of this change is to clean up some of the code surrounding
HLSL using CXXThisExpr as a non-pointer l-value. This change cleans up
a bunch of assumptions and inconsistencies around how the type of
this is handled through the AST and code generation.

This change is be mostly NFC for HLSL, and completely NFC for other
language modes.

This change introduces a new member to query for the this object's type
and seeks to clarify the normal usages of the this type.

With the introudction of HLSL to clang, CXXThisExpr may now be an
l-value and behave like a reference type rather than C++'s normal
method of it being an r-value of pointer type.

With this change there are now three ways in which a caller might need
to query the type of this:

  • The type of the CXXThisExpr
  • The type of the object this referrs to
  • The type of the implicit (or explicit) this argument

This change codifies those three ways you may need to query
respectively as:

  • CXXMethodDecl::getThisType()
  • CXXMethodDecl::getThisObjectType()
  • CXXMethodDecl::getThisArgType()

This change then revisits all uses of getThisType(), and in cases
where the only use was to resolve the pointee type, it replaces the
call with getThisObjectType(). In other cases it evaluates whether
the desired returned type is the type of the this expr, or the type
of the this function argument. The this expr type is used for
creating additional expr AST nodes and for member lookup, while the
argument type is used mostly for code generation.

Additionally some cases that used getThisType in simple queries could
be substituted for getThisObjectType. Since getThisType is
implemented in terms of getThisObjectType calling the later should be
more efficient if the former isn't needed.

Diff Detail

Event Timeline

beanz created this revision.Aug 30 2023, 7:11 PM
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
beanz requested review of this revision.Aug 30 2023, 7:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 7:11 PM
beanz added a comment.Aug 30 2023, 7:13 PM

Full disclosure here, I need to write some more tests for this. It should be NFC for all languages other than HLSL, but I want to spend some time making sure that the tests adequately exercise the HLSL paths (which should also be mostly NFC).

Thanks for working on this.
This is going to conflict with deducing this (https://reviews.llvm.org/D140828) in interesting (ie, horrible) ways, so i guess I'll have to deal with that.
For people following along we are trying to fix nasty lambda regression https://reviews.llvm.org/D159126, for which the present patch is needed so we probably needs to land the current patch before deducing this, which is less than ideal.

I do wonder if we do really need 3 functions though.

We ought to have this being either a pointer (C++), or a reference (HLSL), and the type of the object it points to.
It's unclear to me that we could not just have 2 methods.

  • getThisObjectType
  • getThisType (as implemented in getThisArgType)

What would happen if we would:

  • Make getThisType return a reference type (for HLSL)
  • Explicitely remove the reference where we need to

If that would be too verbose, maybe renaming would add some clarity
getThisReferenceType and getThisType - this would mirror what we do for deducing this (getFunctionObjectParameterReferenceType and getFunctionObjectParameterType)

Ultimately we can have:

  • This being a pointer in C++
  • This a reference in HLSL
  • The object parameter being a reference or a pointer depending on whether it is explicit or not in C++23, or depending on whether we are using HLSL

Which is... not confusing at all !

Yea, we could do that approach. It will mean filtering about a few getNonReferenceType() calls, which is what I was trying to avoid. That said, it might be the better solution since it can be unconditional and will work for both deducing this and HLSL.

I'll test it out and post an update shortly.

beanz updated this revision to Diff 555118.Aug 31 2023, 11:31 AM

Updating based on review feedback from @core3ntin. Thank you!

I would like @aaron.ballman to weight in, (he might not get back to you until next week), but overall, i think I'm pretty happy with this new direction.

clang/include/clang/AST/DeclCXX.h
2174 ↗(On Diff #555118)

Whitespace only change

shafik added inline comments.Aug 31 2023, 1:08 PM
clang/include/clang/AST/DeclCXX.h
2174 ↗(On Diff #555118)

Spurious change.

clang/lib/Sema/SemaExprMember.cpp
1901–1902

To be consistent with bugprone-argument-comment

clang/lib/Sema/SemaTemplate.cpp
763–766
beanz updated this revision to Diff 555380.Sep 1 2023, 7:56 AM

Updating based on PR feedback.

aaron.ballman accepted this revision.Sep 5 2023, 5:20 AM

LGTM, I like this as a simplification, too!

This revision is now accepted and ready to land.Sep 5 2023, 5:20 AM
bogner accepted this revision.Sep 5 2023, 2:44 PM
This revision was landed with ongoing or failed builds.Sep 5 2023, 5:49 PM
This revision was automatically updated to reflect the committed changes.