[llvm] r267232 - Revert r267210, it makes clang assert (PR27490).
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 22 20:58:48 PDT 2016
@Hans - Thanks. That saved me a ton of time.
As expected, the problem was really obvious once I got it into the
debugger. I'll resubmit a fixed patch (minus an assert, plus comments)
on Monday. A description of the conceptual problem is now on the bug.
Philip
On 04/22/2016 03:58 PM, Hans Wennborg wrote:
> I've attached an IR repro to the bug.
>
> Thanks,
> Hans
>
> On Fri, Apr 22, 2016 at 3:37 PM, Philip Reames via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>> Also, I'm assuming this is https://llvm.org/bugs/show_bug.cgi?id=27490? If
>> so, could you please attach an IR reproducer? I appreciate the C
>> reproducer, but I do not have a recent build of clang. I work only on LLVM.
>>
>> Philip
>>
>>
>> On 04/22/2016 03:35 PM, Philip Reames via llvm-commits wrote:
>>> Nico,
>>>
>>> I have no objection to this being reverted, but in the future, please make
>>> sure to reply to the original commit thread so that the author knows
>>> something is going on. There was a discussion on IRC, but I got drug into
>>> that quite late and email is our system of record.
>>>
>>> Philip
>>>
>>> On 04/22/2016 03:08 PM, Nico Weber via llvm-commits wrote:
>>>> Author: nico
>>>> Date: Fri Apr 22 17:08:42 2016
>>>> New Revision: 267232
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=267232&view=rev
>>>> Log:
>>>> Revert r267210, it makes clang assert (PR27490).
>>>>
>>>> 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=267232&r1=267231&r2=267232&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>>> (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>>> Fri Apr 22 17:08:42 2016
>>>> @@ -327,8 +327,6 @@ static LoadInst *combineLoadToNewType(In
>>>> LoadInst *NewLoad = IC.Builder->CreateAlignedLoad(
>>>> IC.Builder->CreateBitCast(Ptr, NewTy->getPointerTo(AS)),
>>>> LI.getAlignment(), LI.getName() + Suffix);
>>>> - NewLoad->setAtomic(LI.getOrdering(), LI.getSynchScope());
>>>> - assert(!LI.isVolatile() && "volatile unhandled here");
>>>> MDBuilder MDB(NewLoad->getContext());
>>>> for (const auto &MDPair : MD) {
>>>> unsigned ID = MDPair.first;
>>>> @@ -401,8 +399,6 @@ static StoreInst *combineStoreToNewValue
>>>> StoreInst *NewStore = IC.Builder->CreateAlignedStore(
>>>> V, IC.Builder->CreateBitCast(Ptr,
>>>> V->getType()->getPointerTo(AS)),
>>>> SI.getAlignment());
>>>> - NewStore->setAtomic(SI.getOrdering(), SI.getSynchScope());
>>>> - assert(!SI.isVolatile() && "volatile unhandled here");
>>>> for (const auto &MDPair : MD) {
>>>> unsigned ID = MDPair.first;
>>>> MDNode *N = MDPair.second;
>>>> @@ -460,9 +456,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
>>>> ordered
>>>> - // atomic loads here but it isn't clear that this is important.
>>>> - if (!LI.isUnordered())
>>>> + // 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())
>>>> return nullptr;
>>>> if (LI.use_empty())
>>>> @@ -896,7 +892,6 @@ Instruction *InstCombiner::visitLoadInst
>>>> V1->setAtomic(LI.getOrdering(), LI.getSynchScope());
>>>> V2->setAlignment(Align);
>>>> V2->setAtomic(LI.getOrdering(), LI.getSynchScope());
>>>> - assert(!LI.isVolatile() && "volatile unhandled here");
>>>> return SelectInst::Create(SI->getCondition(), V1, V2);
>>>> }
>>>> @@ -939,9 +934,9 @@ Instruction *InstCombiner::visitLoadInst
>>>> /// 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
>>>> ordered
>>>> - // atomic stores here but it isn't clear that this is important.
>>>> - if (!SI.isUnordered())
>>>> + // 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())
>>>> 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=267232&r1=267231&r2=267232&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/InstCombine/atomic.ll (original)
>>>> +++ llvm/trunk/test/Transforms/InstCombine/atomic.ll Fri Apr 22 17:08:42
>>>> 2016
>>>> @@ -173,45 +173,6 @@ define i32 @test17(i1 %cnd) {
>>>> ret i32 %x
>>>> }
>>>> -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
>>>> -}
>>>> -; TODO: probably also legal in this case
>>>> -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 i32 @test22(i1 %cnd) {
>>>> ; CHECK-LABEL: define i32 @test22(
>>>> ; CHECK: [[PHI:%.*]] = phi i32
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> 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
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list