[PATCH] D128012: [HLSL] Add ExternalSemaSource & vector alias

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D128012#3630680 <https://reviews.llvm.org/D128012#3630680>, @beanz wrote:

> @aaron.ballman, I just had a total brain-asleep moment, and pushed this without realizing that I had feedback from you. Are you okay with me addressing that most-commit or would you prefer a revert? Huge apologies...

No worries, it happens! Post-commit is fine by me; no need to have significant churn.



================
Comment at: clang/lib/Headers/hlsl/hlsl_basic_types.h:30
 #ifdef __HLSL_ENABLE_16_BIT
-typedef int16_t int16_t2 __attribute__((ext_vector_type(2)));
-typedef int16_t int16_t3 __attribute__((ext_vector_type(3)));
-typedef int16_t int16_t4 __attribute__((ext_vector_type(4)));
-typedef uint16_t uint16_t2 __attribute__((ext_vector_type(2)));
-typedef uint16_t uint16_t3 __attribute__((ext_vector_type(3)));
-typedef uint16_t uint16_t4 __attribute__((ext_vector_type(4)));
+typedef vector<int16_t, 2> int16_t2;
+typedef vector<int16_t, 3> int16_t3;
----------------
beanz wrote:
> aaron.ballman wrote:
> > Can you explain these changes in light of:
> > 
> > > The problem is that templates aren't supported before HLSL 2021, and type aliases still aren't supported in HLSL.
> > 
> > This still looks like it's using template and type aliases.
> This is an extreme total hack of insanity, but it is how HLSL works.
> 
> Basically, user-defined templates aren't supported, but built-in templates have been around for years. In DXC, the `vector` template is a class containing an ext_vector but there are horrible hacks in member expr handling to make the `.` operator return ext_vector swizzles... Using an alias allows me to get source compatibility here without having to do any of the member expr hacking.
Interesting and horrifying in equal measures. :-)


================
Comment at: clang/lib/Sema/HLSLExternalSemaSource.cpp:41-44
+  auto *UsingDecl = UsingDirectiveDecl::Create(
+      AST, AST.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
+      NestedNameSpecifierLoc(), SourceLocation(), HLSLNamespace,
+      AST.getTranslationUnitDecl());
----------------
beanz wrote:
> aaron.ballman wrote:
> > And users won't find this behavior surprising? I would be worried that the user has their own globally declared type that this hidden using directive will then cause to be ambiguous: https://godbolt.org/z/jMsW54vWe -- when users hit this themselves, the location of the conflict is going to point into nowhere-land, which is also rather unfortunate.
> > 
> > Actually, the more I think about this, the less comfortable I am with it. This also means you have to be *very* careful about conflicts with STL names, and it means that any new declarations added to the `hlsl` namespace automatically risk breaking code.
> > 
> > Aside: any particular reason the namespace declaration is implicit but the using directive declaration is not?
> As HLSL is implanted today in DXC the built-in types and functions are all globally defined (not in any namespace). My goal here was to start nesting them in an `hlsl` namespace, and to eventually do that for DXC as well in a new language version. HLSL just got support for `using namespace` like 3 months ago, so we can't do that in a header for compatibility, so my intent was to throw it in the directive here. 
> 
> My hope is the next version of HLSL will move the builtins into the `hlsl` namespace so I can make this only enabled for older language versions in the near future.
> 
> In terms of conflicts with STL, I don't think the STL sources will compile for HLSL anytime soon. Maybe in a future version of HLSL, but we've got a long way to go to get there. For existing HLSL code, namespaces are very sparingly used, and while we may find conflicts in user code I think moving HLSL's builtins into a namespace will be worth the pain... but I might be wrong about how much pain that will be.
So basically this trades redefinition errors for ambiguous lookup errors in terms of conflicts with user-declared code?

> In terms of conflicts with STL, I don't think the STL sources will compile for HLSL anytime soon. Maybe in a future version of HLSL, but we've got a long way to go to get there. For existing HLSL code, namespaces are very sparingly used, and while we may find conflicts in user code I think moving HLSL's builtins into a namespace will be worth the pain... but I might be wrong about how much pain that will be.

I think moving them into a namespace is a lovely idea in theory, but it's still discomforting to add the `using` directive automatically for the user. The user is going to have the pain of dealing with the names moving to a new namespace at some point, so why not start from a cleaner design? It seems not entirely unreasonable to expect users to type `using namespace hlsl;` themselves if you're moving the types into a namespace, and users who need to handle older compilers can use the preprocessor to conditionally compile that line out. Or is that not really a viable option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128012/new/

https://reviews.llvm.org/D128012