[llvm] r322125 - [IPSCCP] Remove calls without side effects
Chris Bieneman via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 11 09:22:31 PST 2018
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> 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 <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/98f5865a/attachment.html>
More information about the llvm-commits
mailing list