InstCombine: Preserve tbaa metadata when merging loads that are phi arguments

[ahatanak]

Summary

This patch attaches the merged metadata to the load instruction that is newly created in InstCombiner::FoldPHIArgLoadIntoPHI.

[ahatanak]

ping

[reames]

lib/Transforms/InstCombine/InstCombinePHI.cpp

357 ↗

(On Diff #34272)

On first look, this seems like it's the wrong approach. You're mutating the metadata on an existing load to accumulate the intersection of all of the incoming loads. It's not clear to me that we want to be killing the metadata on the input load.

[ahatanak]

I was counting on the fact that the incoming loads will eventually get eliminated as they all have one use, so mutating the metadata is safe. But it's probably better to take an approach that doesn't mutate the metadata of an existing load. In the new patch, the metadata is attached to the newly created load.

[hfinkel]

lib/Transforms/InstCombine/InstCombinePHI.cpp

355 ↗

(On Diff #35029)

Why are you preserving only TBAA? I think that you could also preserve here:

MD_alias_scope
MD_noalias
MD_invariant_load
MD_nonnull

using combineMetadata.

[ahatanak]

lib/Transforms/InstCombine/InstCombinePHI.cpp

355 ↗

(On Diff #35029)

Yes, I think you are right. I didn't look at the other metadata since tbaa was the only one that I had to fix for my use case.

Is there a reason you left out the other two (range and fpmath) that are combined in combineMetadata? It seems to me that they can be attached to load instructions and should be preserved too.

[hfinkel]

lib/Transforms/InstCombine/InstCombinePHI.cpp

355 ↗

(On Diff #35029)

I don't think that fpmath applies to loads, but range does, and yes, you should include that too.

[ahatanak]

Preserve the following metadata in addition to tbaa:

MD_alias_scope
MD_noalias
MD_invariant_load
MD_nonnull
MD_range

[hfinkel]

LGTM.

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

 // This file implements the visitPHINode function.
 //
 //===----------------------------------------------------------------------===//
 
 #include "InstCombineInternal.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/Analysis/InstructionSimplify.h"
+#include "llvm/Transforms/Utils/Local.h"
 using namespace llvm;
 
 #define DEBUG_TYPE "instcombine"
 
 /// If we have something like phi [add (a,b), add(a,c)] and if a/b/c and the
 /// adds all have a single use, turn this into a phi and a single binop.
 Instruction *InstCombiner::FoldPHIArgBinOpIntoPHI(PHINode &PN) {
   Instruction *FirstInst = cast<Instruction>(PN.getIncomingValue(0));
Instruction *InstCombiner::FoldPHIArgLoadIntoPHI(PHINode &PN) {
   // Okay, they are all the same operation.  Create a new PHI node of the
   // correct type, and PHI together all of the LHS's of the instructions.
   PHINode *NewPN = PHINode::Create(FirstLI->getOperand(0)->getType(),
                                    PN.getNumIncomingValues(),
                                    PN.getName()+".in");
 
   Value *InVal = FirstLI->getOperand(0);
   NewPN->addIncoming(InVal, PN.getIncomingBlock(0));
+  LoadInst *NewLI = new LoadInst(NewPN, "", isVolatile, LoadAlignment);
 
-  // Add all operands to the new PHI.
+  unsigned KnownIDs[] = {
+    LLVMContext::MD_tbaa,
+    LLVMContext::MD_range,
+    LLVMContext::MD_invariant_load,
+    LLVMContext::MD_alias_scope,
+    LLVMContext::MD_noalias,
+    LLVMContext::MD_nonnull
+  };
+
+  for (unsigned ID : KnownIDs)
+    NewLI->setMetadata(ID, FirstLI->getMetadata(ID));
+
+  // Add all operands to the new PHI and combine TBAA metadata.
   for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) {
-    Value *NewInVal = cast<LoadInst>(PN.getIncomingValue(i))->getOperand(0);
+    LoadInst *LI = cast<LoadInst>(PN.getIncomingValue(i));
+    combineMetadata(NewLI, LI, KnownIDs);
+    Value *NewInVal = LI->getOperand(0);
     if (NewInVal != InVal)
       InVal = nullptr;
     NewPN->addIncoming(NewInVal, PN.getIncomingBlock(i));
   }
 
-  Value *PhiVal;
   if (InVal) {
     // The new PHI unions all of the same values together.  This is really
     // common, so we handle it intelligently here for compile-time speed.
-    PhiVal = InVal;
+    NewLI->setOperand(0, InVal);
     delete NewPN;
   } else {
     InsertNewInstBefore(NewPN, PN);
-    PhiVal = NewPN;
   }
 
   // If this was a volatile load that we are merging, make sure to loop through
   // and mark all the input loads as non-volatile.  If we don't do this, we will
   // insert a new volatile load and the old ones will not be deletable.
   if (isVolatile)
     for (Value *IncValue : PN.incoming_values())
       cast<LoadInst>(IncValue)->setVolatile(false);
 
-  LoadInst *NewLI = new LoadInst(PhiVal, "", isVolatile, LoadAlignment);
   NewLI->setDebugLoc(FirstLI->getDebugLoc());
   return NewLI;
 }
 
 
 
 /// If all operands to a PHI node are the same "unary" operator and they all are
 /// only used by the PHI, PHI together their inputs, and do the operation once,

llvm/trunk/test/Transforms/InstCombine/fold-phi-load-metadata.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+@g1 = common global i32* null, align 8
+
+%struct.S1 = type { i32, float }
+%struct.S2 = type { float, i32 }
+
+; Check that instcombine preserves metadata when it merges two loads.
+;
+; CHECK: return:
+; CHECK: load i32*, i32** %{{[a-z0-9.]+}}, align 8, !nonnull ![[EMPTYNODE:[0-9]+]]
+; CHECK: load i32, i32* %{{[a-z0-9.]+}}, align 4, !tbaa ![[TBAA:[0-9]+]], !range ![[RANGE:[0-9]+]], !invariant.load ![[EMPTYNODE:[0-9]+]], !alias.scope ![[ALIAS_SCOPE:[0-9]+]], !noalias ![[NOALIAS:[0-9]+]]
+
+; Function Attrs: nounwind ssp uwtable
+define i32 @phi_load_metadata(%struct.S1* %s1, %struct.S2* %s2, i32 %c, i32** %x0, i32 **%x1) #0 {
+entry:
+  %tobool = icmp eq i32 %c, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:                                          ; preds = %entry
+  %i = getelementptr inbounds %struct.S2, %struct.S2* %s2, i64 0, i32 1
+  %val = load i32, i32* %i, align 4, !tbaa !0, !alias.scope !13, !noalias !14, !invariant.load !17, !range !18
+  %p0 = load i32*, i32** %x0, align 8, !nonnull !17
+  br label %return
+
+if.end:                                           ; preds = %entry
+  %i2 = getelementptr inbounds %struct.S1, %struct.S1* %s1, i64 0, i32 0
+  %val2 = load i32, i32* %i2, align 4, !tbaa !2, !alias.scope !15, !noalias !16, !invariant.load !17, !range !19
+  %p1 = load i32*, i32** %x1, align 8, !nonnull !17
+  br label %return
+
+return:                                           ; preds = %if.end, %if.then
+  %retval = phi i32 [ %val, %if.then ], [ %val2, %if.end ]
+  %pval = phi i32* [ %p0, %if.then ], [ %p1, %if.end ]
+  store i32* %pval, i32** @g1, align 8
+  ret i32 %retval
+}
+
+; CHECK: ![[EMPTYNODE]] = !{}
+; CHECK: ![[TBAA]] = !{![[TAG1:[0-9]+]], ![[TAG1]], i64 0}
+; CHECK: ![[TAG1]] = !{!"int", !{{[0-9]+}}, i64 0}
+; CHECK: ![[RANGE]] = !{i32 10, i32 25}
+; CHECK: ![[ALIAS_SCOPE]] = !{![[SCOPE0:[0-9]+]], ![[SCOPE1:[0-9]+]], ![[SCOPE2:[0-9]+]]}
+; CHECK: ![[SCOPE0]] = distinct !{![[SCOPE0]], !{{[0-9]+}}, !"scope0"}
+; CHECK: ![[SCOPE1]] = distinct !{![[SCOPE1]], !{{[0-9]+}}, !"scope1"}
+; CHECK: ![[SCOPE2]] = distinct !{![[SCOPE2]], !{{[0-9]+}}, !"scope2"}
+; CHECK: ![[NOALIAS]] = !{![[SCOPE3:[0-9]+]]}
+; CHECK: ![[SCOPE3]] = distinct !{![[SCOPE3]], !{{[0-9]+}}, !"scope3"}
+
+!0 = !{!1, !4, i64 4}
+!1 = !{!"", !7, i64 0, !4, i64 4}
+!2 = !{!3, !4, i64 0}
+!3 = !{!"", !4, i64 0, !7, i64 4}
+!4 = !{!"int", !5, i64 0}
+!5 = !{!"omnipotent char", !6, i64 0}
+!6 = !{!"Simple C/C++ TBAA"}
+!7 = !{!"float", !5, i64 0}
+!8 = !{!8, !"some domain"}
+!9 = !{!9, !8, !"scope0"}
+!10 = !{!10, !8, !"scope1"}
+!11 = !{!11, !8, !"scope2"}
+!12 = !{!12, !8, !"scope3"}
+!13 = !{!9, !10}
+!14 = !{!11, !12}
+!15 = !{!9, !11}
+!16 = !{!10, !12}
+!17 = !{}
+!18 = !{i32 10, i32 20}
+!19 = !{i32 15, i32 25}