[PATCH] D112626: Convert float to double on __builtin_dump_struct

[John McCall via Phabricator via cfe-commits]

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:
> 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.
> 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.

That should also not have been committed, but more importantly, it's not actually mutated during normal operation — IIUC, you have to tell the `cl` library to process a command line for any of the `cl::opt`s to be mutated, and otherwise they remain constant.  In contrast, this code may segfault if you have two threads in a process running IR-generation that happen to use this builtin, and the only saving grace is that approximately nobody uses this builtin.


================
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:
> > 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.
> > 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.
> 
> That should also not have been committed, but more importantly, it's not actually mutated during normal operation — IIUC, you have to tell the `cl` library to process a command line for any of the `cl::opt`s to be mutated, and otherwise they remain constant.  In contrast, this code may segfault if you have two threads in a process running IR-generation that happen to use this builtin, and the only saving grace is that approximately nobody uses this builtin.
> 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.

In addition to its need to reproduce all the logic of default argument conversion, this code is doing manual lowering of calls with arbitrary arguments instead of using the target ABI call-emission code.  It also, as mentioned in the FIXME, doesn't properly handle bit-fields.  It's just the wrong basic approach for implementing this feature, and I don't think we should be trying to clean it up around the edges; we should revert and ask for an acceptable implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112626