[llvm] r268809 - Reapply 267210 with fix for PR27490
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 8 14:58:14 PDT 2016
Patch posted for review as: https://reviews.llvm.org/D24365
On 09/08/2016 12:02 PM, Philip Reames via llvm-commits wrote:
>
> I haven't gotten a lot of time to look at this, but I'm thinking about
> two options:
>
> 1) Allow atomic vector types. There's a solid argument that this is a
> useful canonicalization. Adding the lowering in AtomicExpand would be
> fairly straight forward. I think this is the right long term approach.
>
> 2) Disallow conversion to atomic vector types. This is the more
> obvious fix, but the actual code in question is slightly complicated.
> My first look made it seem like this would be the wrong approach, but
> glancing at this again while I write this, I think I was being a bit
> too pessimistic. Preparing a patch that does this seems much less
> invasive than I first thought.
>
> My current plan is to prepare a patch for approach 2. I may get to
> this today. If not, I will make a point of getting this done over the
> weekend. Sorry for the delay.
>
> Philip
>
>
> On 09/07/2016 02:18 PM, Steven Wu wrote:
>> Ping. Is there any update for this?
>>
>>
>>> On Aug 24, 2016, at 9:00 AM, Steven Wu <stevenwu at apple.com
>>> <mailto: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
>>>>>> 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
>>>>>> ==============================================================================
>>>>>> ---
>>>>>> 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
>>>>>> ==============================================================================
>>>>>> --- 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
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> 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/20160908/b5ef3736/attachment.html>
More information about the llvm-commits
mailing list