[llvm] r268809 - Reapply 267210 with fix for PR27490

Steven Wu via llvm-commits llvm-commits at lists.llvm.org
Wed Sep 7 14:18:14 PDT 2016


Ping. Is there any update for this? 


> On Aug 24, 2016, at 9:00 AM, Steven Wu <stevenwu at apple.com> wrote:
> 
>> 
>> On Aug 23, 2016, at 5:02 PM, Philip Reames <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>> 
>> On 08/23/2016 04:21 PM, Steven Wu wrote:
>>> Hi Philip
>>> 
>>> I found a fallout of this commit. After this commit, instcombine can create atomic loads/stores on vector types which will be rejected by the verifier. Here is a patch and reproducer as testcase.
>>> Please let me know what do you think:
>> Can you file a bug for this with your test cases?  I'll look at this tomorrow.
> 
> File PR29121 and I cc you in the bug. 
> 
>> 
>> I'm not sure your patch is the right direction here.  Using atomic vector in the cases you mentioned actually seem reasonable.  Maybe we should update the AtomicExpand pass and verifier to allow them.
>>> 
>>> commit 4cb8f4c4a5f7f92ba8743c23fdc286380dcdbe4d
>>> Author: Steven Wu <stevenwu at apple.com <mailto:stevenwu at apple.com>>
>>> Date:   Tue Aug 23 15:11:29 2016 -0700
>>> 
>>>     [InstCombine] Don't optimize atomic load/store into unsupported types
>>>          According to the bitcode verifier, atomics can only be used on integer, float
>>>     or pointer type. Check the final type before optimize atomic load and store.
>>> 
>>> diff --git a/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>> index 1603278..5dbcc79 100644
>>> --- a/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>> +++ b/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>> @@ -503,10 +503,15 @@ static Instruction *combineLoadToOperationType(InstCombiner &IC, LoadInst &LI) {
>>>    if (LI.hasOneUse())
>>>      if (auto* CI = dyn_cast<CastInst>(LI.user_back())) {
>>>        if (CI->isNoopCast(DL)) {
>>> -        LoadInst *NewLoad = combineLoadToNewType(IC, LI, CI->getDestTy());
>>> -        CI->replaceAllUsesWith(NewLoad);
>>> -        IC.eraseInstFromFunction(*CI);
>>> -        return &LI;
>>> +        // Atomic can only used on integer, pointer or float type.
>>> +        Type *DestTy = CI->getDestTy();
>>> +        if (!LI.isAtomic() || DestTy->isIntegerTy() || DestTy->isPointerTy() ||
>>> +            DestTy->isFloatTy()) {
>>> +          LoadInst *NewLoad = combineLoadToNewType(IC, LI, CI->getDestTy());
>>> +          CI->replaceAllUsesWith(NewLoad);
>>> +          IC.eraseInstFromFunction(*CI);
>>> +          return &LI;
>>> +        }
>>>        }
>>>      }
>>>  @@ -1001,9 +1006,13 @@ static bool combineStoreToValueType(InstCombiner &IC, StoreInst &SI) {
>>>      // Fold away bit casts of the stored value by storing the original type.
>>>    if (auto *BC = dyn_cast<BitCastInst>(V)) {
>>> -    V = BC->getOperand(0);
>>> -    combineStoreToNewValue(IC, SI, V);
>>> -    return true;
>>> +    Type* SrcTy = BC->getSrcTy();
>>> +    if (!SI.isAtomic() || SrcTy->isIntegerTy() || SrcTy->isPointerTy() ||
>>> +        SrcTy->isFloatTy()) {
>>> +      V = BC->getOperand(0);
>>> +      combineStoreToNewValue(IC, SI, V);
>>> +      return true;
>>> +    }
>>>    }
>>>      if (Value *U = likeBitCastFromVector(IC, V)) {
>>> diff --git a/test/Transforms/InstCombine/atomic.ll b/test/Transforms/InstCombine/atomic.ll
>>> index 15c1659..ac9792b 100644
>>> --- a/test/Transforms/InstCombine/atomic.ll
>>> +++ b/test/Transforms/InstCombine/atomic.ll
>>> @@ -267,3 +267,25 @@ define void @pr27490b(i8** %p1, i8** %p2) {
>>>    store atomic i8* %l, i8** %p2 seq_cst, align 8
>>>    ret void
>>>  }
>>> +
>>> +define <2 x float> @no_atomic_vector_load(i8* %p) {
>>> +; CHECK-LABEL: define <2 x float> @no_atomic_vector_load(
>>> +; CHECK-NOT: load atomic <2 x float>
>>> +  %retval = alloca <2 x float>, align 8
>>> +  %1 = bitcast i8* %p to <2 x float>*
>>> +  %2 = bitcast <2 x float>* %1 to i64*
>>> +  %load = load atomic i64, i64* %2 unordered, align 8
>>> +  %3 = bitcast <2 x float>* %retval to i64*
>>> +  store i64 %load, i64* %3, align 8
>>> +  %4 = load <2 x float>, <2 x float>* %retval, align 8
>>> +  ret <2 x float> %4
>>> +}
>>> +
>>> +define void @no_atomic_vector_store(<2 x float> %p, i8* %p2) {
>>> +; CHECK-LABEL: define void @no_atomic_vector_store(
>>> +; CHECK-NOT: store atomic <2 x float>
>>> +  %1 = bitcast <2 x float> %p to i64
>>> +  %2 = bitcast i8* %p2 to i64*
>>> +  store atomic i64 %1, i64* %2 unordered, align 8
>>> +  ret void
>>> +}
>> Ending up with a store atomic <2 x float> seems correct here. What's the reasoning for this being wrong?  Just the verifier failure?
> 
> I don't know the background why the verifier reject this specific type but Legalizer/SelectionDAG certainly cannot handle that. It is possible to update the verifier and atomic expand pass to allow that. Since I am not an export in that area, I am just proposing a safer approach. 
> 
> Thanks
> 
> Steven
> 
>>> 
>>> 
>>> Thanks
>>> 
>>> Steven
>>> 
>>>> On May 6, 2016, at 3:17 PM, Philip Reames via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> Author: reames
>>>> Date: Fri May  6 17:17:01 2016
>>>> New Revision: 268809
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=268809&view=rev <http://llvm.org/viewvc/llvm-project?rev=268809&view=rev>
>>>> Log:
>>>> Reapply 267210 with fix for PR27490
>>>> 
>>>> Original Commit Message
>>>> Extend load/store type canonicalization to handle unordered operations
>>>> 
>>>> Extend the type canonicalization logic to work for unordered atomic loads and stores.  Note that while this change itself is fairly simple and low risk, there's a reasonable chance this will expose problems in the backends by suddenly generating IR they wouldn't have seen before.  Anything of this nature will be an existing bug in the backend (you could write an atomic float load), but this will definitely change the frequency with which such cases are encountered.  If you see problems, feel free to revert this change, but please make sure you collect a test case.
>>>> 
>>>> Note that the concern about lowering is now much less likely.  PR27490 proved that we already *were* mucking with the types of ordered atomics and volatiles.  As a result, this change doesn't introduce as much new behavior as originally thought.
>>>> 
>>>> 
>>>> 
>>>> Modified:
>>>>    llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>>>    llvm/trunk/test/Transforms/InstCombine/atomic.ll
>>>> 
>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=268809&r1=268808&r2=268809&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=268809&r1=268808&r2=268809&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp Fri May  6 17:17:01 2016
>>>> @@ -326,7 +326,8 @@ static LoadInst *combineLoadToNewType(In
>>>> 
>>>>   LoadInst *NewLoad = IC.Builder->CreateAlignedLoad(
>>>>       IC.Builder->CreateBitCast(Ptr, NewTy->getPointerTo(AS)),
>>>> -      LI.getAlignment(), LI.getName() + Suffix);
>>>> +      LI.getAlignment(), LI.isVolatile(), LI.getName() + Suffix);
>>>> +  NewLoad->setAtomic(LI.getOrdering(), LI.getSynchScope());
>>>>   MDBuilder MDB(NewLoad->getContext());
>>>>   for (const auto &MDPair : MD) {
>>>>     unsigned ID = MDPair.first;
>>>> @@ -398,7 +399,8 @@ static StoreInst *combineStoreToNewValue
>>>> 
>>>>   StoreInst *NewStore = IC.Builder->CreateAlignedStore(
>>>>       V, IC.Builder->CreateBitCast(Ptr, V->getType()->getPointerTo(AS)),
>>>> -      SI.getAlignment());
>>>> +      SI.getAlignment(), SI.isVolatile());
>>>> +  NewStore->setAtomic(SI.getOrdering(), SI.getSynchScope());
>>>>   for (const auto &MDPair : MD) {
>>>>     unsigned ID = MDPair.first;
>>>>     MDNode *N = MDPair.second;
>>>> @@ -456,9 +458,9 @@ static StoreInst *combineStoreToNewValue
>>>> /// later. However, it is risky in case some backend or other part of LLVM is
>>>> /// relying on the exact type loaded to select appropriate atomic operations.
>>>> static Instruction *combineLoadToOperationType(InstCombiner &IC, LoadInst &LI) {
>>>> -  // FIXME: We could probably with some care handle both volatile and atomic
>>>> -  // loads here but it isn't clear that this is important.
>>>> -  if (!LI.isSimple())
>>>> +  // FIXME: We could probably with some care handle both volatile and ordered
>>>> +  // atomic loads here but it isn't clear that this is important.
>>>> +  if (!LI.isUnordered())
>>>>     return nullptr;
>>>> 
>>>>   if (LI.use_empty())
>>>> @@ -989,9 +991,9 @@ static Value *likeBitCastFromVector(Inst
>>>> /// the store instruction as otherwise there is no way to signal whether it was
>>>> /// combined or not: IC.EraseInstFromFunction returns a null pointer.
>>>> static bool combineStoreToValueType(InstCombiner &IC, StoreInst &SI) {
>>>> -  // FIXME: We could probably with some care handle both volatile and atomic
>>>> -  // stores here but it isn't clear that this is important.
>>>> -  if (!SI.isSimple())
>>>> +  // FIXME: We could probably with some care handle both volatile and ordered
>>>> +  // atomic stores here but it isn't clear that this is important.
>>>> +  if (!SI.isUnordered())
>>>>     return false;
>>>> 
>>>>   Value *V = SI.getValueOperand();
>>>> 
>>>> Modified: llvm/trunk/test/Transforms/InstCombine/atomic.ll
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/atomic.ll?rev=268809&r1=268808&r2=268809&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/atomic.ll?rev=268809&r1=268808&r2=268809&view=diff>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/InstCombine/atomic.ll (original)
>>>> +++ llvm/trunk/test/Transforms/InstCombine/atomic.ll Fri May  6 17:17:01 2016
>>>> @@ -206,3 +206,64 @@ block2:
>>>> merge:
>>>>   ret i32 0
>>>> }
>>>> +
>>>> +declare void @clobber()
>>>> +
>>>> +define i32 @test18(float* %p) {
>>>> +; CHECK-LABEL: define i32 @test18(
>>>> +; CHECK: load atomic i32, i32* [[A:%.*]] unordered, align 4
>>>> +; CHECK: store atomic i32 [[B:%.*]], i32* [[C:%.*]] unordered, align 4
>>>> +  %x = load atomic float, float* %p unordered, align 4
>>>> +  call void @clobber() ;; keep the load around
>>>> +  store atomic float %x, float* %p unordered, align 4
>>>> +  ret i32 0
>>>> +}
>>>> +
>>>> +; TODO: probably also legal in this case
>>>> +define i32 @test19(float* %p) {
>>>> +; CHECK-LABEL: define i32 @test19(
>>>> +; CHECK: load atomic float, float* %p seq_cst, align 4
>>>> +; CHECK: store atomic float %x, float* %p seq_cst, align 4
>>>> +  %x = load atomic float, float* %p seq_cst, align 4
>>>> +  call void @clobber() ;; keep the load around
>>>> +  store atomic float %x, float* %p seq_cst, align 4
>>>> +  ret i32 0
>>>> +}
>>>> +
>>>> +define i32 @test20(i32** %p, i8* %v) {
>>>> +; CHECK-LABEL: define i32 @test20(
>>>> +; CHECK: store atomic i8* %v, i8** [[D:%.*]] unordered, align 4
>>>> +  %cast = bitcast i8* %v to i32*
>>>> +  store atomic i32* %cast, i32** %p unordered, align 4
>>>> +  ret i32 0
>>>> +}
>>>> +
>>>> +define i32 @test21(i32** %p, i8* %v) {
>>>> +; CHECK-LABEL: define i32 @test21(
>>>> +; CHECK: store atomic i32* %cast, i32** %p monotonic, align 4
>>>> +  %cast = bitcast i8* %v to i32*
>>>> +  store atomic i32* %cast, i32** %p monotonic, align 4
>>>> +  ret i32 0
>>>> +}
>>>> +
>>>> +define void @pr27490a(i8** %p1, i8** %p2) {
>>>> +; CHECK-LABEL: define void @pr27490
>>>> +; CHECK: %1 = bitcast i8** %p1 to i64*
>>>> +; CHECK: %l1 = load i64, i64* %1, align 8
>>>> +; CHECK: %2 = bitcast i8** %p2 to i64*
>>>> +; CHECK: store volatile i64 %l1, i64* %2, align 8
>>>> +  %l = load i8*, i8** %p1
>>>> +  store volatile i8* %l, i8** %p2
>>>> +  ret void
>>>> +}
>>>> +
>>>> +define void @pr27490b(i8** %p1, i8** %p2) {
>>>> +; CHECK-LABEL: define void @pr27490
>>>> +; CHECK: %1 = bitcast i8** %p1 to i64*
>>>> +; CHECK: %l1 = load i64, i64* %1, align 8
>>>> +; CHECK: %2 = bitcast i8** %p2 to i64*
>>>> +; CHECK: store atomic i64 %l1, i64* %2 seq_cst, align 8
>>>> +  %l = load i8*, i8** %p1
>>>> +  store atomic i8* %l, i8** %p2 seq_cst, align 8
>>>> +  ret void
>>>> +}
>>>> 
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160907/41577fae/attachment.html>


More information about the llvm-commits mailing list