rjmccall added inline comments.

================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2090-2094
+    // Variadic functions expect the caller to promote float to double.
+    if (CanonicalType == Context.FloatTy) {
+      FieldPtr =
+          CGF.Builder.CreateFPExt(FieldPtr, CGF.ConvertType(Context.DoubleTy));
+    }
----------------
aaron.ballman wrote:
> This change is an improvement as far as it goes, but I think we might be 
> missing other floating-point promotions here. For example, `__fp16` fields 
> also seem to be unusable: https://godbolt.org/z/z3a45f9YE
> 
> Also, we don't seem to handle the integer promotions at all (but still get 
> correct results there), so I think we're getting the correct behavior there 
> by chance rather than by design. Oh, yeah, note the differences here: 
> https://godbolt.org/z/f13eq3668
> 
> ```
> foo:
>   ...
>   %7 = load i8, i8* %4, align 1, !dbg !217
>   %8 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([6 x i8], [6 x 
> i8]* @2, i32 0, i32 0), i8 %7), !dbg !217
>   ...
> 
> bar:
>   ...
>   %2 = load i8, i8* %1, align 1, !dbg !222
>   %3 = zext i8 %2 to i32, !dbg !222
>   %4 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([5 x i8], [5 x 
> i8]* @.str, i64 0, i64 0), i32 %3), !dbg !223
>   ...
> ```
> I think we should probably fix all of the promotion problems at once rather 
> than piecemeal.
> 
It's actually really annoying that this logic has to be duplicated in IRGen 
instead of being able to take advantage of the existing promotion logic in 
Sema.  Can we just generate a helper function in Sema and somehow link it to 
the builtin call?

Um.  Also, the `static` local DenseMap in the code above this is totally 
unacceptable and should not have been committed.  Clang is supposed to be 
embeddable as a library and should not be using global mutable variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112626

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

Reply via email to