[cfe-commits] r104422 - in /cfe/trunk/lib/CodeGen: CGExprAgg.cpp CGObjC.cpp CGObjCGNU.cpp CGObjCMac.cpp CGObjCRuntime.h CodeGenFunction.h

[Douglas Gregor]

On May 22, 2010, at 9:13 AM, Fariborz Jahanian wrote:

> 
> On May 21, 2010, at 6:48 PM, John McCall wrote:
> 
>> Author: rjmccall
>> Date: Fri May 21 20:48:05 2010
>> New Revision: 104422
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=104422&view=rev
>> Log:
>> Push a return-value slot throughout ObjC message-send codegen.  Will  
>> be
>> critical for ObjC++ correctness;  hard to test independently of  
>> various
>> required Sema changes, though.
>> 
>> 
>> Modified:
>>   cfe/trunk/lib/CodeGen/CGExprAgg.cpp
>>   cfe/trunk/lib/CodeGen/CGObjC.cpp
>>   cfe/trunk/lib/CodeGen/CGObjCGNU.cpp
>>   cfe/trunk/lib/CodeGen/CGObjCMac.cpp
>>   cfe/trunk/lib/CodeGen/CGObjCRuntime.h
>>   cfe/trunk/lib/CodeGen/CodeGenFunction.h
>> 
>> Modified: cfe/trunk/lib/CodeGen/CGExprAgg.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprAgg.cpp?rev=104422&r1=104421&r2=104422&view=diff
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> ======================================================================
>> --- cfe/trunk/lib/CodeGen/CGExprAgg.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGExprAgg.cpp Fri May 21 20:48:05 2010
>> @@ -37,6 +37,10 @@
>>  bool IgnoreResult;
>>  bool IsInitializer;
>>  bool RequiresGCollection;
>> +
>> +  ReturnValueSlot getReturnValueSlot() const {
>> +    return ReturnValueSlot(DestPtr, VolatileDest);
>> +  }
>> public:
>>  AggExprEmitter(CodeGenFunction &cgf, llvm::Value *destPtr, bool v,
>>                 bool ignore, bool isinit, bool requiresGCollection)
>> @@ -299,7 +303,7 @@
>>  // If the struct doesn't require GC, we can just pass the  
>> destination
>>  // directly to EmitCall.
>>  if (!RequiresGCollection) {
>> -    CGF.EmitCallExpr(E, ReturnValueSlot(DestPtr, VolatileDest));
>> +    CGF.EmitCallExpr(E, getReturnValueSlot());
>>    return;
>>  }
>> 
>> @@ -308,19 +312,16 @@
>> }
>> 
>> void AggExprEmitter::VisitObjCMessageExpr(ObjCMessageExpr *E) {
>> -  RValue RV = CGF.EmitObjCMessageExpr(E);
>> -  EmitFinalDestCopy(E, RV);
>> +  CGF.EmitObjCMessageExpr(E, getReturnValueSlot());
>> }
>> 
>> void AggExprEmitter::VisitObjCPropertyRefExpr(ObjCPropertyRefExpr  
>> *E) {
>> -  RValue RV = CGF.EmitObjCPropertyGet(E);
>> -  EmitFinalDestCopy(E, RV);
>> +  CGF.EmitObjCPropertyGet(E, getReturnValueSlot());
>> }
>> 
>> void AggExprEmitter::VisitObjCImplicitSetterGetterRefExpr(
>>                                   ObjCImplicitSetterGetterRefExpr  
>> *E) {
>> -  RValue RV = CGF.EmitObjCPropertyGet(E);
>> -  EmitFinalDestCopy(E, RV);
>> +  CGF.EmitObjCPropertyGet(E, getReturnValueSlot());
>> }
>> 
> 
> 
> Hi John,
> 
> Wouldn't removing call to EmitFinalDestCopy change things in this  
> patch? This routine deals with GC API, as well as copying result to  
> DestPtr.

The problem with EmitFinalDestCopy is that it attempts to memcpy non-POD class types in C++, which is wrong. So, we're moving to a system where we avoid the memcpy's altogether by using the return slot of the function. That's good for performance and for correctness. Why does the GC need to know about this?

> Our clang test has minimal coverage of API issues.

:(

	- Doug
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100522/bc70e3b0/attachment.html>