[LLVMdev] [PATCH] PR2218

[Chris Lattner]

On Sep 2, 2009, at 2:04 PM, Jakub Staszak wrote:
>> Please make this a static function, it doesn't need "this".  Also,  
>> please do something like this in the for loop:
>>
>> +    for (argI = 0, CS.arg_size(); argI != argE; ++argI)
>>
>>
>> pointerIsParameter returning a bool and the argument # byref is  
>> somewhat awkward.  I think it would be better if it returned an  
>> int, which was -1 if not found.
>
> The problem is that CS.arg_size() is "unsigned int". Of course, I  
> can cast int -> unsigned int but it wouldn't look good I think.

The typical way to do this is to return it as int, then at the call  
site do something like:


int V = foo();
if (V == -1) handle error

unsigned RealVal = (unsigned)V;

That way, it is very explicit what is going on.

>> +  } else if (StoreInst *S = dyn_cast<StoreInst>(dep.getInst())) {
>> ...
>> +    uint64_t ptrSize = AA.getTypeStoreSize(pointer->getType());
>> ...
>> +    uint64_t memSize = AA.getTypeStoreSize(memPtr->getType());
>> ...
>> +    if (AA.alias(pointer, ptrSize, memPtr, memSize) !=  
>> AliasAnalysis::MustAlias)
>>
>> This is passing in the size of the pointer, not the pointee.   
>> However, since you only care about must alias, you don't care about  
>> the size of the object, you can just pass in 1 for both sizes.
>
> Ahh, last time you said I had to check sizes. You said:
> "Knowing that the loaded and stored pointer must alias is important,  
> but you also need to check that the loaded and stored sizes equal  
> each other." :-)

This is a different issue.  Previously you would trigger when the load  
and store were of a pointer that could have been bitcast.  You need to  
check that the load and store have the same size, but that doesn't  
impact the alias query.

>> I'm pretty sure that there is still a legality check missing here.   
>> Your code will transform this:
>>
>>  %temporary = alloca i8
>>  call void @initialize(i8* noalias sret %temporary)
>>  %tmp = load i8* %temporary
>>  store i8 42, i8* %result
>>  store i8 %tmp, i8* %result
>>
>> to:
>>
>>  %temporary = alloca i8
>>  call void @initialize(i8* noalias sret %result)
>>  store i8 42, i8* %result
>>
>> which is a miscompilation.  To fix this, I'd do a memdep query of  
>> "result" from StoreOfLoad and from L.  If they both have the same  
>> dependence, then the transform is ok.
>
> See PR2218-dont.ll test.

What do you mean?

-Chris