[InstCombine] Add rule to fold away trunc of partial load

[anna]

Summary

This instcombine rule is created to fold away trunc operations that are generated for partial loads. The trunc can be folded away when the value for partial load is available from a prior store or load.

This kind of code can be generated as a result of GVN widening or in some source code as well. The current rule considers partial loads/stores of the form:

%pload = load i8, i8* %a, align 2
  %wide.load = load i16, i16* %0, align 2
  %lowhalf.1 = trunc i16 %wide.load to i8
  call void @consume(i8 %lowhalf.1)
  call void @consume(i8 %pload).

The store partially feeding into load is added as a test case.

[anna]

The current rule gets the lower part of the load instruction on an LE machine. We could also add rules where we get the upper half of the load (would require a different pattern match in visitTrunc and pass in the offset to FindAvailableLoadedValue).

[sanjoy]

Some minor comments inline.

lib/Analysis/Loads.cpp

388 ↗

(On Diff #60414)

Doesn't the atomicity concern above apply here too?

429 ↗

(On Diff #60414)

Why not use CastInst::isBitOrNoopPointerCastable here as well (same for the load case)?

lib/Transforms/InstCombine/InstCombineCasts.cpp

580 ↗

(On Diff #60414)

Is this clang-formatted ?

test/Transforms/InstCombine/trunc.ll

163 ↗

(On Diff #60414)

Add a store test case, and also one or two negative test cases.

[anna]

addressed above comments.

[sanjoy]

lib/Analysis/Loads.cpp

385 ↗

(On Diff #60522)

It feels odd that we have to special-case PartialType this way. I think a better design would be to introduce a new function FindAvailableMemoryContents(Value *Ptr, Type *AccessTy ... < no PartialTy>) (perhaps with a different name), and have this version of FindAvailableLoadedValue dispatch to FindAvailableMemoryContents one with LI->getPointerOperand(), LI->getType(). visitTrunc can then call FindAvailableMemoryContents directly.

lib/Transforms/InstCombine/InstCombineCasts.cpp

588 ↗

(On Diff #60522)

Here if we have:

store i64 %x to %ptr
%val = load i64* %ptr
%val.tr = trunc i64 %val to i32

won't we have a type mismatch and try to replace %val.tr with %x?

[anna]

lib/Analysis/Loads.cpp

385 ↗

(On Diff #60522)

It seems ok, but are we fine with changing FindAvailableLoadedValue to being a wrapper function, with all the actual code in FindAvailableMemoryContents?

lib/Transforms/InstCombine/InstCombineCasts.cpp

588 ↗

(On Diff #60522)

We won't replace %val.tr with %x since the DestTy is i32. We do the check in FindAvailableLoadedValue (LI: %val,.., DestTy: i32) that i64 is BitorNoopPointerCastable to the PartialType i32.
I've added this test case as well now.

[sanjoy]

lib/Analysis/Loads.cpp

385 ↗

(On Diff #60522)

I don't see any problems with that (you can put them both in this .cpp file for the time being).

lib/Transforms/InstCombine/InstCombineCasts.cpp

588 ↗

(On Diff #60522)

I was thinking of the case where the first check, i.e.

if (AreEquivalentAddressValues(LoadPtr, StrippedPtr) &&
    CastInst::isBitOrNoopPointerCastable(LI->getType(), AccessTy, DL)) {

or

if (AreEquivalentAddressValues(StorePtr, StrippedPtr) &&
    CastInst::isBitOrNoopPointerCastable(SI->getValueOperand()->getType(),
                                         AccessTy, DL)) {

would fire and you'd return a wider value. So the test case has to be

define i1 @trunc_different_size_load(i32 * align 4 %a) {
  store i32 0, i32 *%a, align 4
  %wide.load = load i32, i32* %a, align 4
  %lowhalf = trunc i32 %wide.load to i8
  call void @consume(i8 %lowhalf)
  %cmp.2 = icmp ult i32 %wide.load, 256
  ret i1 %cmp.2
; CHECK-LABEL: @trunc_different_size_load
; CHECK: %lowhalf = trunc i32 %wide.load to i8
}

[anna]

lib/Transforms/InstCombine/InstCombineCasts.cpp

588 ↗

(On Diff #60522)

I see your point about the widening and the mismatch of types. The reason the firing of the first check did not trigger any failures was because a prior instcombine rule (I think it is combineLoadStore which calls FindAvailableLoad as well) already removes the trunc. So with or without this trunc rule in instcombine, opt with -instcombine changes the above test case to:

define i1 @trunc_different_size_loads(i32* align 4 %a) {
  store i32 0, i32* %a, align 4
  call void @consume(i8 0)
  ret i1 false
}

Also verified the value is correctly truncated, say in this case:

store i32 257, i32* %a, align 4
call void @consume(i8 1)
ret i1 false

That said, I agree we should not be relying on a certain ordering of instcombine rules for this, since there would be a type mismatch in absence of load store instcombine rules.
Will make the change. Thanks :)

[anna]

lib/Analysis/Loads.cpp

385 ↗

(On Diff #60522)

Just noticed there is a similar change that went in for the same function checking.

This change adds a new parameter to the function isLoadCSE, with default as nullptr.

I'll have to rethink the design for my change. it makes sense to create a separate function which checks for partial memory contents. However, we do not want to keep adding extra parameters to the FindAvailableLoadedValue function, since this would mean keeping track of the parameters (or atleast knowing what each parameter does) when we call FindAvailableLoadedValue from FindAvailableMemoryContents.

[anna]

Just noticed there is a similar change that went in for the same function checking. This change adds a new parameter to the function isLoadCSE, with default as nullptr.

change in question: http://reviews.llvm.org/D21271

[anna]

Added function FindAvailableMemoryContents, which returns the partial or full value at memory location accessed by load.

[sanjoy]

include/llvm/Analysis/Loads.h

64 ↗

(On Diff #61365)

Nit: newer code does not repeat the function name before comments.

67 ↗

(On Diff #61365)

This is not quite the interface I was thinking of. I was thinking of

Value *FindAvailableMemoryContents(Value *Location, Type *AccessTy,
                                   BasicBlock *BB,
                                   BasicBlock::iterator &ScanFrom,
                                   unsigned MaxInstsToScan,
                                   AliasAnalysis *AA = nullptr,
                                   AAMDNodes *AATags = nullptr,
                                   bool *IsLoadCSE = nullptr);

That is, FindAvailableMemoryContents tries to give a Value * that would be the result of loading a value of type AccessTy from Location (this is how it should be specified too). Whether the Location came from a load instruction of elsewhere is not something it needs to worry about.

lib/Analysis/Loads.cpp

303 ↗

(On Diff #61365)

Nit: remove the \brief -- newer code can rely on doxygen using autobrief.

313 ↗

(On Diff #61365)

Why did you remove this? Isn't this pertinent to FindAvailableMemoryContents as well?

455 ↗

(On Diff #61365)

I think this "partial or complete" term (here and elsewhere) are more confusing than helpful. The invariant here is that FindAvailableMemoryContents will return a value of type AccessTy, whether AccessTy is partial or full from the perspective of the caller is not something FindAvailableMemoryContents should worry about. For instance, with the edited interface that takes a pointer and not a LoadInst, I'd rather we have:

// We want a value covering the whole of the load.
return FindAvailableMemoryContents(Load->getPointerOperand(), Load->getType(), ScanBB, ScanFrom, MaxInstsToScan, AA, AATags, IsLoadCSE);

[anna]

Added function signature changed to Value *Ptr instead of LoadInst *Load. Also name changed from FindMemoryContents to overloaded version of FindAvailableLoadedValue.

My main concern with the signature having the Pointer operand Value *Ptr instead of LoadInst *Load was making sure the atomicity checks would be correct (LI->isAtomic < Load->isAtomic) and the corresponding one in the store inst SI case.

Discussed offline with Sanjoy and we decided to leave the sanity checking to the callers of this function (both atomicity concerns and any other checks needed depending on the nature of transformation done once the value is available).
Calling this function to see if a store value is available: FindAvailableLoadedValue(SI->getPointerOperand(), SI->getType,...) would possibly require careful consideration of atomicity rules.

[sanjoy]

Mostly minor comments.

include/llvm/Analysis/Loads.h

65 ↗

(On Diff #61450)

Nit here and later: \p Ptr

(This is optional) Also, instead of a separate line about AccessTy, I'd just restate this to say "if we have the value at address \p Ptr of type \p AccessTy"

80 ↗

(On Diff #61450)

Describe isAtomicMemOp, and note that we're constant folding an unordered load (actually, maybe that's a better name for this function - FoldLoadFromAddress?).

Also, by convention, it should be IsAtomicMemOp.

82 ↗

(On Diff #61450)

I think "sanity checks" is too vague to be useful. I think it is sufficient to state that we've assumed that we're folding a non-volatile unordered (but potentially atomic) load -- it is understood that people calling this function know what they want. :)

lib/Analysis/Loads.cpp

363 ↗

(On Diff #61450)

This bit of change looks unnecessary now

[anna]

include/llvm/Analysis/Loads.h

80 ↗

(On Diff #61450)

Described IsAtomicMemOp as

IsAtomicMemOp specifies the atomicity of the memory operation that accesses \p *Ptr. We verify atomicity constraints are satisfied when value forwarding from another memory operation that has value \p *Ptr available.

But FoldLoadFromAddress is perhaps a misleading name, since the transformation done by the caller need not be folding of load (explained below).

82 ↗

(On Diff #61450)

I think we will miss some subtle points in that case, and will only be considering one usage of this function.
Basically, we do not make any assumptions on the memory operation, i.e. it need not be unordered non-volatile. This constraint is only required for the call from the FindAvailableLoadedValue(LoadInst *Load,...). We do not need this constraint for the call from InstCombiner::visitTrunc, since we do not fold away the load.
Also, the transformation using the value returned, can be a fold of the load as done in most cases, or fold of the trunc and nothing done to the load.

The idea is that it is up to the caller to decide what sort of memory operations they are interested in (may or may not be an unordered load) and what transformation needs to be done based on the value (may or may not be folding of load).

I thought this would be a very detailed explanation, and decided to state it as 'sanity checks' :)

[sanjoy]

include/llvm/Analysis/Loads.h

82 ↗

(On Diff #61450)

We do not need this constraint for the call from
InstCombiner::visitTrunc, since we do not fold away the load.

I'm not sure if we are okay folding volatile loads, but I realized
(just now) that we're not okay folding atomic loads in
visitTrunc. Our program could have been:

%ptr is initially 0

ThreadA:
  store atomic i32 1, i32* %ptr

ThreadB:
  %prevva = load atomic i16, (bitcast %ptr)
  %val = load atomic i32, i32* %ptr
  print(%val)
  print(trunc(%val))

Right now the program above can print 0, 0 or 1, 1, but after your
change, ThreadB will be:

ThreadB:
  %prevva = load atomic i16, (bitcast %ptr)
  %val = load atomic i32, i32* %ptr
  print(%val)
  print(%prevva)

and will alllow the execution 1, 0 and 0, 1. I think we can fold
atomic loads only if the load has trunc as the only use.

[sanjoy]

include/llvm/Analysis/Loads.h

82 ↗

(On Diff #61450)

That example was not correct, to be precise, this is what I'm talking
about:

%ptr is 0 initially

ThreadA:
  store atomic i32 -1, i32* %ptr

ThreadB:
  %val0 = load atomic i16, i16* (bitcast %ptr)
  %val1 = load atomic i32, i32* %ptr
  %val2 = trunc i32 %val1 to i16
  print(%val1, %val2)

Now ThreadB can print only "0, 0" or "-1, -1", but after you replace
%val2 with %val0, it will be possible for it to print "0, -1" or "-1,
0".

[anna]

include/llvm/Analysis/Loads.h

82 ↗

(On Diff #61450)

Good catch! I initially thought FindAvailableLoadedValue atomicity check may handle the case you've written: if (LI->isAtomic() < isAtomicMemOp) return nullptr, but here both loads are atomic, and this would cause incorrect results only in visitTrunc.

I'll have this check before deciding if visitTrunc can call this function (include volatile check for safety).

if (auto *LI = dyn_cast<LoadInst>(Src)) { // source operand of trunc is load
if(!LI->isVolatile() && (!LI->isAtomic() || LI->hasOneUse()) )
   call FindAvailableLoadedValue
}

and add above example within theadB as testcase to see trunc not removed.

[anna]

Added check in visitTrunc to confirm the load is non-volatile (volatile loads have target dependent results, and I'm not sure if it will affect any targets) and if the load is atomic, it has only a single use, which is the trunc.
Added a test case to verify trunc is not removed in the case of atomic load with more than one use.

[sanjoy]

lgtm

[anna]

The above change caused a build break when the result type of trunc being folded did not match that of the
prior load/store that had the trunc value.

The change was reverted in: r273703 due to fail in Chromium and sanitizer build. This was the bad RAUW assertion:
Assertion failed: New->getType() == getType() && "replaceAllUses of value with new value of different type!", file ..\lib\IR\Value.cpp, line 375

Load being used:

%138 = load float, float* %m_x.i527, align 4, !dbg !447
Trunc being replaced:

%155 = trunc i64 %140 to i32, !dbg !470

The RAUW return replaceInstUsesWith(CI, AvailableVal); was missing Builder->CreateBitOrPointerCast of AvailableVal.

[anna]

All changes will be tracked in new review which addresses the build break: http://reviews.llvm.org/D21791

llvm/trunk/include/llvm/Analysis/Loads.h

bool isSafeToLoadUnconditionally(Value *V, unsigned Align,
                                  Instruction *ScanFrom = nullptr,
                                  const DominatorTree *DT = nullptr,
                                  const TargetLibraryInfo *TLI = nullptr);
 
 /// DefMaxInstsToScan - the default number of maximum instructions
 /// to scan in the block, used by FindAvailableLoadedValue().
 extern cl::opt<unsigned> DefMaxInstsToScan;
 
+/// Scan the ScanBB block backwards checking to see if we have the value at
+/// the memory address \p Ptr of type \p AccessTy locally available within a
+/// small number of instructions. If the value is available, return it.
+///
+/// If not, return the iterator for the last validated instruction that the
+/// value would be live through.  If we scanned the entire block and didn't
+/// find something that invalidates *Ptr or provides it, ScanFrom would be
+/// left at begin() and this returns null.  ScanFrom could also be left
+///
+/// MaxInstsToScan specifies the maximum instructions to scan in the block.
+/// If it is set to 0, it will scan the whole block. You can also optionally
+/// specify an alias analysis implementation, which makes this more precise.
+///
+/// If AATags is non-null and a load or store is found, the AA tags from the
+/// load or store are recorded there.  If there are no AA tags or if no access
+/// is found, it is left unmodified.
+///
+/// IsAtomicMemOp specifies the atomicity of the memory operation that accesses
+/// \p *Ptr. We verify atomicity constraints are satisfied when value forwarding
+/// from another memory operation that has value \p *Ptr available.
+///
+/// Note that we assume the \p *Ptr is accessed through a non-volatile but
+/// potentially atomic load. Any other constraints should be verified at the
+/// caller.
+Value *FindAvailableLoadedValue(Value *Ptr, Type *AccessTy, bool IsAtomicMemOp,
+                                BasicBlock *ScanBB,
+                                BasicBlock::iterator &ScanFrom,
+                                unsigned MaxInstsToScan,
+                                AliasAnalysis *AA = nullptr,
+                                AAMDNodes *AATags = nullptr,
+                                bool *IsLoadCSE = nullptr);
+
 /// FindAvailableLoadedValue - Scan the ScanBB block backwards (starting at
 /// the instruction before ScanFrom) checking to see if we have the value at
 /// the memory address *Ptr locally available within a small number of
 ///  instructions. If the value is available, return it.
 ///
 /// If not, return the iterator for the last validated instruction that the
 /// value would be live through.  If we scanned the entire block and didn't
 /// find something that invalidates *Ptr or provides it, ScanFrom would be

llvm/trunk/lib/Analysis/Loads.cpp

 /// At that point, the value of MaxInstsToScan was already set to '6'
 /// without documented explanation.
 cl::opt<unsigned>
 llvm::DefMaxInstsToScan("available-load-scan-limit", cl::init(6), cl::Hidden,
   cl::desc("Use this to specify the default maximum number of instructions "
            "to scan backward from a given instruction, when searching for "
            "available loaded value"));
 
-/// \brief Scan the ScanBB block backwards to see if we have the value at the
-/// memory address *Ptr locally available within a small number of instructions.
+/// Scan the ScanBB block backwards checking to see if we have the value at
+/// the memory address \p Ptr of type \p AccessTy locally available within a
+/// small number of instructions. If the value is available, return it.
 ///
 /// The scan starts from \c ScanFrom. \c MaxInstsToScan specifies the maximum
 /// instructions to scan in the block. If it is set to \c 0, it will scan the whole
 /// block.
 ///
 /// If the value is available, this function returns it. If not, it returns the
 /// iterator for the last validated instruction that the value would be live
 /// through. If we scanned the entire block and didn't find something that
 /// invalidates \c *Ptr or provides it, \c ScanFrom is left at the last
 /// instruction processed and this returns null.
 ///
 /// You can also optionally specify an alias analysis implementation, which
 /// makes this more precise.
 ///
 /// If \c AATags is non-null and a load or store is found, the AA tags from the
 /// load or store are recorded there. If there are no AA tags or if no access is
-/// found, it is left unmodified.
-Value *llvm::FindAvailableLoadedValue(LoadInst *Load, BasicBlock *ScanBB,
+/// is found, it is left unmodified.
+///
+/// IsAtomicMemOp specifies the atomicity of the memory operation that accesses
+/// \p *Ptr. We verify atomicity constraints are satisfied when value forwarding
+/// from another memory operation that has value \p *Ptr available.
+///
+/// Note that we assume the \p *Ptr is accessed through a non-volatile but
+/// potentially atomic load. Any other constraints should be verified at the
+/// caller.
+Value *llvm::FindAvailableLoadedValue(Value *Ptr, Type *AccessTy, bool IsAtomicMemOp,
+                                      BasicBlock *ScanBB,
                                       BasicBlock::iterator &ScanFrom,
                                       unsigned MaxInstsToScan,
                                       AliasAnalysis *AA, AAMDNodes *AATags,
                                       bool *IsLoadCSE) {
+
   if (MaxInstsToScan == 0)
     MaxInstsToScan = ~0U;
 
-  Value *Ptr = Load->getPointerOperand();
-  Type *AccessTy = Load->getType();
-
-  // We can never remove a volatile load
-  if (Load->isVolatile())
-    return nullptr;
-
-  // Anything stronger than unordered is currently unimplemented.
-  if (!Load->isUnordered())
-    return nullptr;
-
   const DataLayout &DL = ScanBB->getModule()->getDataLayout();
 
   // Try to get the store size for the type.
   uint64_t AccessSize = DL.getTypeStoreSize(AccessTy);
 
   Value *StrippedPtr = Ptr->stripPointerCasts();
 
   while (ScanFrom != ScanBB->begin()) {
while (ScanFrom != ScanBB->begin()) {
     // Don't scan huge blocks.
     if (MaxInstsToScan-- == 0)
       return nullptr;
 
     --ScanFrom;
     // If this is a load of Ptr, the loaded value is available.
     // (This is true even if the load is volatile or atomic, although
     // those cases are unlikely.)
-    if (LoadInst *LI = dyn_cast<LoadInst>(Inst))
-      if (AreEquivalentAddressValues(
-              LI->getPointerOperand()->stripPointerCasts(), StrippedPtr) &&
+    if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
+      Value *LoadPtr = LI->getPointerOperand()->stripPointerCasts();
+      if (AreEquivalentAddressValues(LoadPtr, StrippedPtr) &&
           CastInst::isBitOrNoopPointerCastable(LI->getType(), AccessTy, DL)) {
 
         // We can value forward from an atomic to a non-atomic, but not the
         // other way around.
-        if (LI->isAtomic() < Load->isAtomic())
+        if (LI->isAtomic() < IsAtomicMemOp)
           return nullptr;
 
         if (AATags)
           LI->getAAMetadata(*AATags);
         if (IsLoadCSE)
             *IsLoadCSE = true;
         return LI;
       }
 
+    }
+
     if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       Value *StorePtr = SI->getPointerOperand()->stripPointerCasts();
       // If this is a store through Ptr, the value is available!
       // (This is true even if the store is volatile or atomic, although
       // those cases are unlikely.)
       if (AreEquivalentAddressValues(StorePtr, StrippedPtr) &&
           CastInst::isBitOrNoopPointerCastable(SI->getValueOperand()->getType(),
                                                AccessTy, DL)) {
 
         // We can value forward from an atomic to a non-atomic, but not the
         // other way around.
-        if (SI->isAtomic() < Load->isAtomic())
+        if (SI->isAtomic() < IsAtomicMemOp)
           return nullptr;
 
         if (AATags)
           SI->getAAMetadata(*AATags);
         return SI->getOperand(0);
       }
 
       // If both StrippedPtr and StorePtr reach all the way to an alloca or
if (Inst->mayWriteToMemory()) {
       ++ScanFrom;
       return nullptr;
     }
   }
 
   // Got to the start of the block, we didn't find it, but are done for this
   // block.
   return nullptr;
+
+}
+
+/// \brief Scan the ScanBB block backwards to see if we have the value at the
+/// memory address *Ptr locally available within a small number of instructions.
+///
+/// The scan starts from \c ScanFrom. \c MaxInstsToScan specifies the maximum
+/// instructions to scan in the block. If it is set to \c 0, it will scan the whole
+/// block.
+///
+/// If the value is available, this function returns it. If not, it returns the
+/// iterator for the last validated instruction that the value would be live
+/// through. If we scanned the entire block and didn't find something that
+/// invalidates \c *Ptr or provides it, \c ScanFrom is left at the last
+/// instruction processed and this returns null.
+///
+/// You can also optionally specify an alias analysis implementation, which
+/// makes this more precise.
+///
+/// If \c AATags is non-null and a load or store is found, the AA tags from the
+/// load or store are recorded there. If there are no AA tags or if no access is
+/// is found, it is left unmodified.
+Value *llvm::FindAvailableLoadedValue(LoadInst *Load, BasicBlock *ScanBB,
+                                      BasicBlock::iterator &ScanFrom,
+                                      unsigned MaxInstsToScan,
+                                      AliasAnalysis *AA, AAMDNodes *AATags,
+                                      bool *IsLoadCSE) {
+
+  // We can never remove a volatile load
+  if (Load->isVolatile())
+    return nullptr;
+
+  // Anything stronger than unordered is currently unimplemented.
+  if (!Load->isUnordered())
+    return nullptr;
+
+  // Return the full value of the load if available.
+  return FindAvailableLoadedValue(Load->getPointerOperand(), Load->getType(),
+                                  Load->isAtomic(), ScanBB, ScanFrom,
+                                  MaxInstsToScan, AA, AATags, IsLoadCSE);
 }

llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp

 //===- InstCombineCasts.cpp -----------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the visit functions for cast operations.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InstCombineInternal.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/Analysis/ConstantFolding.h"
+#include "llvm/Analysis/Loads.h"
+#include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/PatternMatch.h"
-#include "llvm/Analysis/TargetLibraryInfo.h"
 using namespace llvm;
 using namespace PatternMatch;
 
 #define DEBUG_TYPE "instcombine"
 
 /// Analyze 'Val', seeing if it is a simple linear expression.
 /// If so, decompose it, returning some value X, such that Val is
 /// X*Scale+Offset.
if (Src->hasOneUse() && isa<IntegerType>(SrcTy) &&
     Value *NewTrunc = Builder->CreateTrunc(A, DestTy, A->getName() + ".tr");
     return BinaryOperator::CreateAnd(NewTrunc,
                                      ConstantExpr::getTrunc(Cst, DestTy));
   }
 
   if (Instruction *I = foldVecTruncToExtElt(CI, *this, DL))
     return I;
 
+  // When trunc operand is a widened load, see if we can get the value from a
+  // previous store/load
+  if (auto *LI = dyn_cast<LoadInst>(Src)) {
+    BasicBlock::iterator BBI(*LI);
+
+    // Scan a few instructions up from LI and if we find a partial load/store
+    // of Type DestTy that feeds into LI, we can replace all uses of the trunc
+    // with the load/store value.
+    // This replacement can be done only in the case of non-volatile loads. If
+    // the load is atomic, its only use should be the trunc instruction. We
+    // don't want to allow other users of LI to see a value that is out of sync
+    // with the value we're folding the trunc to (in case of a race).
+    if (!LI->isVolatile() && (!LI->isAtomic() || LI->hasOneUse()))
+      if (Value *AvailableVal = FindAvailableLoadedValue(
+              LI->getPointerOperand(), DestTy, LI->isAtomic(), LI->getParent(),
+              BBI, DefMaxInstsToScan))
+        return replaceInstUsesWith(CI, AvailableVal);
+  }
   return nullptr;
 }
 
 /// Transform (zext icmp) to bitwise / integer operations in order to eliminate
 /// the icmp.
 Instruction *InstCombiner::transformZExtICmp(ICmpInst *ICI, Instruction &CI,
                                              bool DoXform) {
   // If we are just checking for a icmp eq of a single bit and zext'ing it

llvm/trunk/test/Transforms/InstCombine/trunc.ll

 bb1:
   %tmp5 = sub i32 0, %tmp1
   %tmp6 = sub i32 %tmp5, 1
   unreachable
 
 bb2:
   unreachable
 }
+
+declare void @consume(i8) readonly
+define i1 @trunc_load_store(i8* align 2 %a) {
+  store i8 0, i8 *%a, align 2
+  %bca  = bitcast i8* %a to i16*
+  %wide.load = load i16, i16* %bca, align 2
+  %lowhalf.1 = trunc i16 %wide.load to i8
+  call void @consume(i8 %lowhalf.1)
+  %cmp.2 = icmp ult i16 %wide.load, 256
+  ret i1 %cmp.2
+; CHECK-LABEL: @trunc_load_store
+; CHECK-NOT: trunc
+; CHECK: call void @consume(i8 0)
+}
+
+
+; The trunc can be replaced with the load value.
+define i1 @trunc_load_load(i8* align 2 %a) {
+  %pload = load i8, i8* %a, align 2
+  %bca  = bitcast i8* %a to i16*
+  %wide.load = load i16, i16* %bca, align 2
+  %lowhalf = trunc i16 %wide.load to i8
+  call void @consume(i8 %lowhalf)
+  call void @consume(i8 %pload)
+  %cmp.2 = icmp ult i16 %wide.load, 256
+  ret i1 %cmp.2
+; CHECK-LABEL: @trunc_load_load
+; CHECK-NOT: trunc
+}
+
+; trunc should not be replaced since atomic load %wide.load has more than one use.
+; different values can be seen by the uses of %wide.load in case of race.
+define i1 @trunc_atomic_loads(i8* align 2 %a) {
+  %pload = load atomic i8, i8* %a unordered, align 2
+  %bca  = bitcast i8* %a to i16*
+  %wide.load = load atomic i16, i16* %bca unordered, align 2
+  %lowhalf = trunc i16 %wide.load to i8
+  call void @consume(i8 %lowhalf)
+  call void @consume(i8 %pload)
+  %cmp.2 = icmp ult i16 %wide.load, 256
+  ret i1 %cmp.2
+; CHECK-LABEL: @trunc_atomic_loads
+; CHECK: trunc
+}
+
+; trunc cannot be replaced since store size is not trunc result size
+define i1 @trunc_different_size_load(i16 * align 2 %a) {
+  store i16 0, i16 *%a, align 2
+  %bca  = bitcast i16* %a to i32*
+  %wide.load = load i32, i32* %bca, align 2
+  %lowhalf = trunc i32 %wide.load to i8
+  call void @consume(i8 %lowhalf)
+  %cmp.2 = icmp ult i32 %wide.load, 256
+  ret i1 %cmp.2
+; CHECK-LABEL: @trunc_different_size_load
+; CHECK: %lowhalf = trunc i32 %wide.load to i8
+}