[llvm] r244244 - [Reassociation] Fix miscompile for va_arg arguments.
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Sun Aug 9 14:07:35 PDT 2015
Hello Quentin,
This has a large impact on our internal non-regression performance suite, I’m not sure it is expected. According to the commit message I would expect this commit to only affect va_arg (which we don’t have). So I have the impression that a “big hammer” was used here with larger impact that intended.
Can you comment on that?
—
Mehdi
> On Aug 6, 2015, at 11:44 AM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: qcolombet
> Date: Thu Aug 6 13:44:34 2015
> New Revision: 244244
>
> URL: http://llvm.org/viewvc/llvm-project?rev=244244&view=rev
> Log:
> [Reassociation] Fix miscompile for va_arg arguments.
>
> iisUnmovableInstruction() had a list of instructions hardcoded which are
> considered unmovable. The list lacked (at least) an entry for the va_arg
> and cmpxchg instructions.
> Fix this by introducing a new Instruction::mayBeMemoryDependent()
> instead of maintaining another instruction list.
>
> Patch by Matthias Braun <matze at braunis.de>.
>
> Differential Revision: http://reviews.llvm.org/D11577
>
> rdar://problem/22118647
>
> Added:
> llvm/trunk/test/Transforms/Reassociate/vaarg_movable.ll
> Modified:
> llvm/trunk/include/llvm/Analysis/ValueTracking.h
> llvm/trunk/lib/Analysis/ValueTracking.cpp
> llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/ValueTracking.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ValueTracking.h?rev=244244&r1=244243&r2=244244&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/ValueTracking.h (original)
> +++ llvm/trunk/include/llvm/Analysis/ValueTracking.h Thu Aug 6 13:44:34 2015
> @@ -258,6 +258,16 @@ namespace llvm {
> const DominatorTree *DT = nullptr,
> const TargetLibraryInfo *TLI = nullptr);
>
> + /// Returns true if the result or effects of the given instructions \p I
> + /// depend on or influence global memory.
> + /// Memory dependence arises for example if the the instruction reads from
> + /// memory or may produce effects or undefined behaviour. Memory dependent
> + /// instructions generally cannot be reorderd with respect to other memory
> + /// dependent instructions or moved into non-dominated basic blocks.
> + /// Instructions which just compute a value based on the values of their
> + /// operands are not memory dependent.
> + bool mayBeMemoryDependent(const Instruction &I);
> +
> /// isKnownNonNull - Return true if this pointer couldn't possibly be null by
> /// its definition. This returns true for allocas, non-extern-weak globals
> /// and byval arguments.
>
> Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=244244&r1=244243&r2=244244&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Thu Aug 6 13:44:34 2015
> @@ -3161,6 +3161,10 @@ bool llvm::isSafeToSpeculativelyExecute(
> }
> }
>
> +bool llvm::mayBeMemoryDependent(const Instruction &I) {
> + return I.mayReadOrWriteMemory() || !isSafeToSpeculativelyExecute(&I);
> +}
> +
> /// Return true if we know that the specified value is never null.
> bool llvm::isKnownNonNull(const Value *V, const TargetLibraryInfo *TLI) {
> // Alloca never returns null, malloc might.
>
> Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=244244&r1=244243&r2=244244&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Thu Aug 6 13:44:34 2015
> @@ -26,6 +26,7 @@
> #include "llvm/ADT/STLExtras.h"
> #include "llvm/ADT/SetVector.h"
> #include "llvm/ADT/Statistic.h"
> +#include "llvm/Analysis/ValueTracking.h"
> #include "llvm/IR/CFG.h"
> #include "llvm/IR/Constants.h"
> #include "llvm/IR/DerivedTypes.h"
> @@ -255,27 +256,6 @@ static BinaryOperator *isReassociableOp(
> return nullptr;
> }
>
> -static bool isUnmovableInstruction(Instruction *I) {
> - switch (I->getOpcode()) {
> - case Instruction::PHI:
> - case Instruction::LandingPad:
> - case Instruction::Alloca:
> - case Instruction::Load:
> - case Instruction::Invoke:
> - case Instruction::UDiv:
> - case Instruction::SDiv:
> - case Instruction::FDiv:
> - case Instruction::URem:
> - case Instruction::SRem:
> - case Instruction::FRem:
> - return true;
> - case Instruction::Call:
> - return !isa<DbgInfoIntrinsic>(I);
> - default:
> - return false;
> - }
> -}
> -
> void Reassociate::BuildRankMap(Function &F) {
> unsigned i = 2;
>
> @@ -295,7 +275,7 @@ void Reassociate::BuildRankMap(Function
> // we cannot move. This ensures that the ranks for these instructions are
> // all different in the block.
> for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I)
> - if (isUnmovableInstruction(I))
> + if (mayBeMemoryDependent(*I))
> ValueRankMap[&*I] = ++BBRank;
> }
> }
>
> Added: llvm/trunk/test/Transforms/Reassociate/vaarg_movable.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Reassociate/vaarg_movable.ll?rev=244244&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/Reassociate/vaarg_movable.ll (added)
> +++ llvm/trunk/test/Transforms/Reassociate/vaarg_movable.ll Thu Aug 6 13:44:34 2015
> @@ -0,0 +1,28 @@
> +; RUN: opt -S -reassociate -die < %s | FileCheck %s
> +
> +; The two va_arg instructions depend on the memory/context, are therfore not
> +; identical and the sub should not be optimized to 0 by reassociate.
> +;
> +; CHECK-LABEL @func(
> +; ...
> +; CHECK: %v0 = va_arg i8** %varargs, i32
> +; CHECK: %v1 = va_arg i8** %varargs, i32
> +; CHECK: %v0.neg = sub i32 0, %v0
> +; CHECK: %sub = add i32 %v0.neg, 1
> +; CHECK: %add = add i32 %sub, %v1
> +; ...
> +; CHECK: ret i32 %add
> +define i32 @func(i32 %dummy, ...) {
> + %varargs = alloca i8*, align 8
> + %varargs1 = bitcast i8** %varargs to i8*
> + call void @llvm.va_start(i8* %varargs1)
> + %v0 = va_arg i8** %varargs, i32
> + %v1 = va_arg i8** %varargs, i32
> + %sub = sub nsw i32 %v1, %v0
> + %add = add nsw i32 %sub, 1
> + call void @llvm.va_end(i8* %varargs1)
> + ret i32 %add
> +}
> +
> +declare void @llvm.va_start(i8*)
> +declare void @llvm.va_end(i8*)
>
>
> _______________________________________________
> 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