python3kgae added inline comments.

================
Comment at: clang/lib/Basic/LangOptions.cpp:197
 
-  // OpenCL has half keyword
-  Opts.Half = Opts.OpenCL;
+  // OpenCL and HLSL have half keyword
+  Opts.Half = Opts.OpenCL || Opts.HLSL;
----------------
aaron.ballman wrote:
> beanz wrote:
> > aaron.ballman wrote:
> > > python3kgae wrote:
> > > > python3kgae wrote:
> > > > > aaron.ballman wrote:
> > > > > > python3kgae wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > Shouldn't this be looking for HLSL 2018? Or shader model 6.2?
> > > > > > > half keyword is always available.
> > > > > > > Without enable_16bit_types, half will be like using half=float.
> > > > > > > With enable_16bit_types, half will be real half.
> > > > > > > 
> > > > > > > The check for HLSL 2018 and shader model 6.2 will be in another 
> > > > > > > PR, still WIP. I'll add FIXME about it.
> > > > > > > half keyword is always available.
> > > > > > > Without enable_16bit_types, half will be like using half=float.
> > > > > > > With enable_16bit_types, half will be real half.
> > > > > > 
> > > > > > Is there room for change here, or is this strictly required by 
> > > > > > HLSL? This strikes me as just begging to confuse users into 
> > > > > > creating ODR violations. CC @beanz 
> > > > > Here's the doc about half for dxc.
> > > > > https://github.com/microsoft/DirectXShaderCompiler/wiki/16-Bit-Scalar-Types
> > > > > 
> > > > > Old doc for fxc (the old shader compiler for shader model <= 5.1) is 
> > > > > here
> > > > > https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-scalar
> > > > > 
> > > > > Change the behavior might affect a lot of existing shaders.
> > > > More detail about half from https://github.com/tex3d.
> > > > 
> > > > half originally mapped to a fuzzy "partial precision" float, where some 
> > > > operations were designated as _pp, meaning the implementation was free 
> > > > to use lower-precision math for those operations (like 24-bit, but not 
> > > > specified for the language). All storage in host-visible memory would 
> > > > still be float. "partial precision" went away with DX9, eventually to 
> > > > be replaced with min-precision with a specific minimum precision an 
> > > > implementation was allowed to use. When "partial precision" went away, 
> > > > it simply mapped to float for DX10+.
> > > > People could have tried to use it liberally when they thought 32-bit 
> > > > precision wasn't necessary, without explicitly targeting/testing 
> > > > API/hardware that actually supported lower precision.
> > > Thank you for the extra information, it sounds like we're stuck 
> > > supporting this.
> > Because HLSL’s library linking mode is pretty constrained, in practice this 
> > hasn’t hurt us yet. That said, I think we probably should consider some 
> > retroactive changes to how we handle float-16, especially in the presence 
> > of library shaders…
> > 
> > I’ll try and float this topic in some team meetings this week and see if we 
> > can come up with a set of tweaks that may have limited impact on existing 
> > code. We might be able to change the default behavior in clang with a 
> > switch to toggle back to the old mode for legacy code.
> > I’ll try and float this topic in some team meetings this week and see if we 
> > can come up with a set of tweaks that may have limited impact on existing 
> > code. We might be able to change the default behavior in clang with a 
> > switch to toggle back to the old mode for legacy code.
> 
> Thanks, both for the awesome pun ("float this topic", lol) and for checking 
> on this. Let's hold off on this patch until we hear more on whether we want 
> to change the default behavior in Clang here or not.
Finally got this discussed today.
The result is we should do the same thing as dxc for back-compat.
And that will require half has a special name when mangling even when half is 
16bit type.
Will update the PR shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124790

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to