[llvm] r322125 - [IPSCCP] Remove calls without side effects
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 10 15:36:27 PST 2018
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. 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/Instruction.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/Transforms/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/Transforms/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/Transforms/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
>>>
>>>
>
>
More information about the llvm-commits
mailing list