[llvm] r322125 - [IPSCCP] Remove calls without side effects
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 11 11:36:25 PST 2018
Thanks!
On 01/11/2018 09:22 AM, Chris Bieneman wrote:
> I will work up a patch to adapt to Philip's suggestion.
>
> -Chris
>
>> On Jan 11, 2018, at 8:47 AM, Daniel Berlin via llvm-commits
>> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>>
>>
>> On Wed, Jan 10, 2018 at 3:36 PM, Philip Reames via
>> llvm-commits<llvm-commits at lists.llvm.org
>> <mailto: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
>> <mailto: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
>> <mailto: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
>> <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
>> <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
>> <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
>> <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
>> <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
>> <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
>> <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
>> <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>>
>> _______________________________________________
>> 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
>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>>
>>
>>
>>
>>
>> _______________________________________________
>> 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
>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>>
>>
>> _______________________________________________
>> 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/20180111/d8c95756/attachment-0001.html>
More information about the llvm-commits
mailing list