[llvm] r322125 - [IPSCCP] Remove calls without side effects

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 11 08:47:53 PST 2018 

On Wed, Jan 10, 2018 at 3:36 PM, Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> I think this is wrong.  In particular, an ordered memory operation is NOT
> safe to remove during constant propagation, even if it's result is known.
> The ordered load has a side effect of ordering other loads which is not
> safe to remove.  The entire *point* of having side effects is that there's
> an effect other than the result of the instruction.
>
> Now, looking at the code I see that this is not a bug this change
> introduced.


Yes, my only goal was to keep the same semantics that existed prior to this
change.
If we think these semantics are wrong, ...


> It appears to have been present in the code previously.  (Well, if
> tryToReplaceWithConstant could even return true for said side effecting
> instructions.)
>
> I'll point out that another call site to the same function exists for the
> same purpose in the same file. This is clearly not the first time this has
> come up.
>
> I am now specifically asking that this be changed to use
> isInstructionTriviallyDead which accounts properly for these cases, or a
> clear test case where that is not sufficient be provided.
>
> Philip
>
>
>
> On 01/10/2018 07:22 AM, Davide Italiano wrote:
>
>> Please refer to the review for details.
>> Copy/pasting Danny's comment here for your convenience:
>>
>> "That would not return true for anything with side effects. So it will
>> not handle the atomic loads and stores cases. mayHaveSideEffects will
>> return true for anything that might write to memory. mayWriteToMemory
>> will return true for any store, and any ordered load.
>>
>> In fact, even getModRefInfo wouldn't help you with that here, it would
>> do the same, and can't be overridden by anything more powerful right
>> now (we only have an AA interface for callsites, the regular
>> instructions are all single implementation in AliasAnalysis.cpp)
>>
>> So TL; DR if you wanted to use isInstructionTriviallyDead, it would be
>> something like isInstructionTriviallyDead || is a load || is a store
>> || atomicrmw || cmpxchng || ...."
>>
>> --
>> Davide
>>
>> On Tue, Jan 9, 2018 at 10:40 PM, Philip Reames
>> <listmail at philipreames.com> wrote:
>>
>>> I think I'm missing something then.  Why would any trivially dead
>>> instruction not be safe to remove in SCCP after we've done the constant
>>> folding of the result?
>>>
>>> Philip
>>>
>>>
>>>
>>> On 01/09/2018 04:09 PM, Davide Italiano wrote:
>>>
>>>> hmm, the original proposal was that of keeping this function private
>>>> to SCCP as it has slightly different requirement
>>>> (`isInstructionTriviallyDead()` wasn't enough in this case).
>>>> Anyway, happy either way (if this gets merged as Philip suggested).
>>>>
>>>> On Tue, Jan 9, 2018 at 4:01 PM, Philip Reames via llvm-commits
>>>> <llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> This appears to be duplicating functionality from
>>>>> isInstructionTriviallyDead.  Please merge them.
>>>>>
>>>>> Philip
>>>>>
>>>>>
>>>>>
>>>>> On 01/09/2018 01:58 PM, Chris Bieneman via llvm-commits wrote:
>>>>>
>>>>>> Author: cbieneman
>>>>>> Date: Tue Jan  9 13:58:46 2018
>>>>>> New Revision: 322125
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=322125&view=rev
>>>>>> Log:
>>>>>> [IPSCCP] Remove calls without side effects
>>>>>>
>>>>>> Summary:
>>>>>> When performing constant propagation for call instructions we have
>>>>>> historically replaced all uses of the return from a call, but not
>>>>>> removed
>>>>>> the call itself. This is required for correctness if the calls have
>>>>>> side
>>>>>> effects, however the compiler should be able to safely remove calls
>>>>>> that
>>>>>> don't have side effects.
>>>>>>
>>>>>> This allows the compiler to completely fold away calls to functions
>>>>>> that
>>>>>> have no side effects if the inputs are constant and the output can be
>>>>>> determined at compile time.
>>>>>>
>>>>>> Reviewers: davide, sanjoy, bruno, dberlin
>>>>>>
>>>>>> Subscribers: llvm-commits
>>>>>>
>>>>>> Differential Revision: https://reviews.llvm.org/D38856
>>>>>>
>>>>>> Added:
>>>>>>        llvm/trunk/test/Transforms/IPConstantProp/remove-call-inst.ll
>>>>>>          - copied, changed from r322124,
>>>>>> llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll
>>>>>> Modified:
>>>>>>        llvm/trunk/include/llvm/IR/Instruction.h
>>>>>>        llvm/trunk/lib/IR/Instruction.cpp
>>>>>>        llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
>>>>>>
>>>>>> llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll
>>>>>>
>>>>>> Modified: llvm/trunk/include/llvm/IR/Instruction.h
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>>>>>> IR/Instruction.h?rev=322125&r1=322124&r2=322125&view=diff
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/include/llvm/IR/Instruction.h (original)
>>>>>> +++ llvm/trunk/include/llvm/IR/Instruction.h Tue Jan  9 13:58:46 2018
>>>>>> @@ -535,6 +535,14 @@ public:
>>>>>>       /// matters, isSafeToSpeculativelyExecute may be more
>>>>>> appropriate.
>>>>>>       bool mayHaveSideEffects() const { return mayWriteToMemory() ||
>>>>>> mayThrow(); }
>>>>>>     +  /// Return true if the instruction can be removed if the
>>>>>> result is
>>>>>> unused.
>>>>>> +  ///
>>>>>> +  /// When constant folding some instructions cannot be removed even
>>>>>> if
>>>>>> their
>>>>>> +  /// results are unused. Specifically terminator instructions and
>>>>>> calls
>>>>>> that
>>>>>> +  /// may have side effects cannot be removed without semantically
>>>>>> changing the
>>>>>> +  /// generated program.
>>>>>> +  bool isSafeToRemove() const;
>>>>>> +
>>>>>>       /// Return true if the instruction is a variety of EH-block.
>>>>>>       bool isEHPad() const {
>>>>>>         switch (getOpcode()) {
>>>>>>
>>>>>> Modified: llvm/trunk/lib/IR/Instruction.cpp
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instru
>>>>>> ction.cpp?rev=322125&r1=322124&r2=322125&view=diff
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/lib/IR/Instruction.cpp (original)
>>>>>> +++ llvm/trunk/lib/IR/Instruction.cpp Tue Jan  9 13:58:46 2018
>>>>>> @@ -589,6 +589,11 @@ bool Instruction::mayThrow() const {
>>>>>>       return isa<ResumeInst>(this);
>>>>>>     }
>>>>>>     +bool Instruction::isSafeToRemove() const {
>>>>>> +  return (!isa<CallInst>(this) || !this->mayHaveSideEffects()) &&
>>>>>> +         !isa<TerminatorInst>(this);
>>>>>> +}
>>>>>> +
>>>>>>     bool Instruction::isAssociative() const {
>>>>>>       unsigned Opcode = getOpcode();
>>>>>>       if (isAssociative(Opcode))
>>>>>>
>>>>>> Modified: llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>>>>>> s/Scalar/SCCP.cpp?rev=322125&r1=322124&r2=322125&view=diff
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
>>>>>> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Tue Jan  9 13:58:46
>>>>>> 2018
>>>>>> @@ -1900,7 +1900,7 @@ static bool runIPSCCP(Module &M, const D
>>>>>>             if (Inst->getType()->isVoidTy())
>>>>>>               continue;
>>>>>>             if (tryToReplaceWithConstant(Solver, Inst)) {
>>>>>> -          if (!isa<CallInst>(Inst) && !isa<TerminatorInst>(Inst))
>>>>>> +          if (Inst->isSafeToRemove())
>>>>>>                 Inst->eraseFromParent();
>>>>>>               // Hey, we just changed something!
>>>>>>               MadeChanges = true;
>>>>>>
>>>>>> Copied: llvm/trunk/test/Transforms/IPConstantProp/remove-call-inst.ll
>>>>>> (from r322124,
>>>>>> llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll)
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transfor
>>>>>> ms/IPConstantProp/remove-call-inst.ll?p2=llvm/trunk/test/
>>>>>> Transforms/IPConstantProp/remove-call-inst.ll&p1=llvm/
>>>>>> trunk/test/Transforms/IPConstantProp/user-with-
>>>>>> multiple-uses.ll&r1=322124&r2=322125&rev=322125&view=diff
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple
>>>>>> -uses.ll
>>>>>> (original)
>>>>>> +++ llvm/trunk/test/Transforms/IPConstantProp/remove-call-inst.ll Tue
>>>>>> Jan
>>>>>> 9 13:58:46 2018
>>>>>> @@ -6,7 +6,7 @@
>>>>>>       ; CHECK: define i32 @main() #0 {
>>>>>>     ; CHECK-NEXT: entry:
>>>>>> -; CHECK-NEXT: %call2 = tail call i32 @wwrite(i64 0) [[NUW:#[0-9]+]]
>>>>>> +; CHECK-NOT: call
>>>>>>     ; CHECK-NEXT: ret i32 123
>>>>>>       define i32 @main() noreturn nounwind {
>>>>>> @@ -31,4 +31,3 @@ return:
>>>>>>       ; CHECK: attributes #0 = { noreturn nounwind }
>>>>>>     ; CHECK: attributes #1 = { nounwind readnone }
>>>>>> -; CHECK: attributes [[NUW]] = { nounwind }
>>>>>>
>>>>>> Modified:
>>>>>> llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll
>>>>>> URL:
>>>>>>
>>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transfor
>>>>>> ms/IPConstantProp/user-with-multiple-uses.ll?rev=322125&
>>>>>> r1=322124&r2=322125&view=diff
>>>>>>
>>>>>>
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple
>>>>>> -uses.ll
>>>>>> (original)
>>>>>> +++ llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple
>>>>>> -uses.ll
>>>>>> Tue Jan  9 13:58:46 2018
>>>>>> @@ -15,7 +15,7 @@ entry:
>>>>>>       ret i32 %call2
>>>>>>     }
>>>>>>     -define internal i32 @wwrite(i64 %i) nounwind readnone {
>>>>>> +define internal i32 @wwrite(i64 %i) nounwind {
>>>>>>     entry:
>>>>>>       switch i64 %i, label %sw.default [
>>>>>>         i64 3, label %return
>>>>>> @@ -30,5 +30,4 @@ return:
>>>>>>     }
>>>>>>       ; CHECK: attributes #0 = { noreturn nounwind }
>>>>>> -; CHECK: attributes #1 = { nounwind readnone }
>>>>>> -; CHECK: attributes [[NUW]] = { nounwind }
>>>>>> +; CHECK: attributes #1 = { nounwind }
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180111/f05291f6/attachment.html>

More information about the llvm-commits mailing list