beanz added inline comments.

================
Comment at: clang/include/clang/Driver/Options.td:6130
   MarshallingInfoFlag<LangOpts<"HalfArgsAndReturns">>,
   ImpliedByAnyOf<[fnative_half_arguments_and_returns.KeyPath]>;
 def fdefault_calling_conv_EQ : Joined<["-"], "fdefault-calling-conv=">,
----------------
bob80905 wrote:
> In Clang.cpp, you removed 
> CmdArgs.push_back("-fallow-half-arguments-and-returns");
> But here, you add an extra condition to imply 
> fnative-half-arguments-and-returns
> Wouldn't this change the behavior? or is it really alright for the definition 
>  below, fallow_half_arguments_and_returns , to be missing hlsl.KeyPath in the 
> implication list?
The big issue with the change in Clang.cpp is that it only works if you invoke 
clang-dxc, not if you invoke clang directly with an hlsl source. That has the 
undesirable effect of making clang HLSL support only work through the clang-dxc 
driver, which isn't what we want.

There is also a subtle difference between native and non-native half arguments 
and returns, where non-native half arguments and returns are actually passed as 
larger float types. Which also isn't correct for HLSL.

This change follows how OpenCL works, and enables 
`-fnative-half-arguments-and-returns` whenever the HLSL language mode is 
enabled, and that option also implies `-fhalf-arguments-and-returns`.

The big functional change here is that you don't need to ever manually pass 
these flags to clang if your language is HLSL, so you can use any clang driver 
entry and get the correct behavior for the language.


================
Comment at: clang/test/CodeGenHLSL/builtins/abs.hlsl:20
+
+// CHECK: define noundef double @"?abs_double@@YANN@Z"(
+// CHECK: call double @llvm.fabs.f64(double %0)
----------------
bob80905 wrote:
> Is there a reason why we don't check the function parameter here, but we do 
> check for function parameters when actually calling the builtin intrinsic on 
> the next call line? 
> 
The check lines on the function declarations are really just to make sure that 
the next match occurs in the function body. The real test is to make sure the 
body gets generated correctly.

That said, matching the full mangled name has the same impact as matching the 
parameters too because the name mangling encodes the parameters too. The `NN` 
specifies the return and first argument parameters are doubles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131718

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

Reply via email to