aaron.ballman 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));
+ }
----------------
rjmccall wrote:
> 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.
> 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?
That seems worth a shot but I think it is not something that needs to happen
for this patch (that could be a rather heavy lift to ask someone who just
joined the community) unless the basic fix starts getting out of hand.
> 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.
We do that with some degree of frequency already (command line arguments using
`llvm::cl::opt` are all global mutable variables, such as
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CoverageMappingGen.cpp#L34),
but yeah, fixing that up would also not be a bad idea, but orthogonal to this
patch IMO.
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
