[llvm-commits] [llvm] r80038 - in /llvm/trunk: include/llvm/InstrTypes.h include/llvm/Instruction.h include/llvm/Instructions.h include/llvm/Value.h lib/Transforms/IPO/MergeFunctions.cpp lib/Transforms/Scalar/InstructionCombining.cpp lib/Transforms/Utils/BasicBlockUtils.cpp lib/Transforms/Utils/SimplifyCFG.cpp lib/VMCore/Instruction.cpp lib/VMCore/Instructions.cpp

[Dan Gohman]

On Aug 25, 2009, at 10:54 PM, Nick Lewycky wrote:


> Dan Gohman wrote:
>
>> Author: djg
>>
>> Date: Tue Aug 25 17:11:20 2009
>>
>> New Revision: 80038
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=80038&view=rev
>>
>> Log:
>>
>> Rename Instruction::isIdenticalTo to  
>> Instruction::isIdenticalToWhenDefined,
>>
>> and introduce a new Instruction::isIdenticalTo which tests for full
>>
>> identity, including the SubclassOptionalData flags. Also, fix the
>>
>> Instruction::clone implementations to preserve the  
>> SubclassOptionalData
>>
>> flags. Finally, teach several optimizations how to handle
>>
>> SubclassOptionalData correctly, given these changes.
>>
>> This fixes the counterintuitive behavior of isIdenticalTo not  
>> comparing
>>
>> the full value, and clone not returning an identical clone, as well  
>> as
>>
>> some subtle bugs that could be caused by these.
>>
>
> Aha! Thanks for spotting that and coming up with such clean solution!
>
>
>> Modified: llvm/trunk/include/llvm/Instructions.h
>>
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Instructions.h?rev=80038&r1=80037&r2=80038&view=diff
>>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>>
>> --- llvm/trunk/include/llvm/Instructions.h (original)
>>
>> +++ llvm/trunk/include/llvm/Instructions.h Tue Aug 25 17:11:20 2009
>>
>> @@ -99,7 +99,6 @@
>>
>> /// MallocInst - an instruction to allocated memory on the heap
>>
>> ///
>>
>> class MallocInst : public AllocationInst {
>>
>> -  MallocInst(const MallocInst &MI);
>>
>
> Why remove these?

I put most the SubclassOptionalData copies in the clone methods, and
it's cleaner to not have two different ways to copy an Instruction,
especially if they work differently.

>
>
>> UnreachableInst *UnreachableInst::clone(LLVMContext &C) const {
>>
>> -  return new UnreachableInst(C);
>>
>> +  UnreachableInst *New = new UnreachableInst(C);
>>
>> +  New->SubclassOptionalData = SubclassOptionalData;
>>
>> +  return New;
>>
>> }
>>
>
> Most of these (ie. unreachable, unwind, invoke, switch) don't have  
> extra bits. Why make copies of SubclassOptionalData? They could add  
> it later if they need it.

I wanted to be complete about it. There will likely be more flags
defined in the future.

>
> Are we taking this into 2.6?

Yes, I'll recommend this patch for 2.6.

Dan