[llvm] r244244 - [Reassociation] Fix miscompile for va_arg arguments.

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 10 09:12:25 PDT 2015


Hi Mehdi,

1. What this means is that the mayBeMemoryDependent does not cover the exact same set of instructions that were defined in the unmovable function in Reassociation, i.e., we should check that. However, there may not be a bug, i.e., Reassociation should probably not reorder those instructions. 
2. What this may also mean is that we were lucky and the performance improvement you were seeing are not semantically valid transformation.
Either way, let us know if we can help. I believe checking #1 is possible on our side but for #2 I have no idea how to reproduce your performances.

Cheers,
-Quentin

> On Aug 9, 2015, at 2:07 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
> 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