[llvm] r290844 - [InstCombine] use combineMetadataForCSE instead of copying it; NFCI

[Sanjay Patel via llvm-commits]

That's what the code comment would have us believe, but... :)

define i8* @isa_impl_wrap(i8** %x) {
  %t2 = alloca i8*
  %t4 = load i8*, i8** %x, !nonnull !0
  store i8* %t4, i8** %t2
  %t6 = load i8*, i8** %t2
  ret i8* %t6
}
!0 = !{}

$ ./opt -instcombine cse_nonnull.ll -S
...
define i8* @isa_impl_wrap(i8** %x) {
  %t4 = load i8*, i8** %x, align 8, !nonnull !0 <--- nonnull preserved when
earlier instruction has it
  ret i8* %t4
}


On Tue, Jan 3, 2017 at 8:53 AM, Davide Italiano <davide at freebsd.org> wrote:

> On Tue, Jan 3, 2017 at 7:21 AM, Sanjay Patel <spatel at rotateright.com>
> wrote:
> >
> >
> > On Tue, Jan 3, 2017 at 4:29 AM, Davide Italiano <davide at freebsd.org>
> wrote:
> >>
> >> On Mon, Jan 2, 2017 at 3:25 PM, Sanjay Patel via llvm-commits
> >> <llvm-commits at lists.llvm.org> wrote:
> >> > Author: spatel
> >> > Date: Mon Jan  2 17:25:28 2017
> >> > New Revision: 290844
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=290844&view=rev
> >> > Log:
> >> > [InstCombine] use combineMetadataForCSE instead of copying it; NFCI
> >> >
> >> > Modified:
> >> >     llvm/trunk/lib/Transforms/InstCombine/
> InstCombineLoadStoreAlloca.cpp
> >> >
> >> > Modified:
> >> > llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> >> > URL:
> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/InstCombine/InstCombineLoadStoreAlloca.
> cpp?rev=290844&r1=290843&r2=290844&view=diff
> >> >
> >> > ============================================================
> ==================
> >> > --- llvm/trunk/lib/Transforms/InstCombine/
> InstCombineLoadStoreAlloca.cpp
> >> > (original)
> >> > +++ llvm/trunk/lib/Transforms/InstCombine/
> InstCombineLoadStoreAlloca.cpp
> >> > Mon Jan  2 17:25:28 2017
> >> > @@ -850,20 +850,10 @@ Instruction *InstCombiner::visitLoadInst
> >> >    // separated by a few arithmetic operations.
> >> >    BasicBlock::iterator BBI(LI);
> >> >    bool IsLoadCSE = false;
> >> > -  if (Value *AvailableVal =
> >> > -      FindAvailableLoadedValue(&LI, LI.getParent(), BBI,
> >> > -                               DefMaxInstsToScan, AA, &IsLoadCSE)) {
> >> > -    if (IsLoadCSE) {
> >> > -      LoadInst *NLI = cast<LoadInst>(AvailableVal);
> >> > -      unsigned KnownIDs[] = {
> >> > -          LLVMContext::MD_tbaa,
> LLVMContext::MD_alias_scope,
> >> > -          LLVMContext::MD_noalias,         LLVMContext::MD_range,
> >> > -          LLVMContext::MD_invariant_load,  LLVMContext::MD_nonnull,
> >> > -          LLVMContext::MD_invariant_group, LLVMContext::MD_align,
> >> > -          LLVMContext::MD_dereferenceable,
> >> > -          LLVMContext::MD_dereferenceable_or_null};
> >> > -      combineMetadata(NLI, &LI, KnownIDs);
> >> > -    };
> >> > +  if (Value *AvailableVal = FindAvailableLoadedValue(
> >> > +          &LI, LI.getParent(), BBI, DefMaxInstsToScan, AA,
> &IsLoadCSE))
> >> > {
> >> > +    if (IsLoadCSE)
> >> > +      combineMetadataForCSE(cast<LoadInst>(AvailableVal), &LI);
> >> >
> >>
> >> We have still a lot of places where we have fundamentally the same'ish
> >> version of this code in the tree
> >> (GVN/GVNHoist/SimplifyCFG/BBVectorize), is it possible to convert to
> >> `combineMetadataForCSE` (or there's some caveat here I'm missing)?
> >
> >
> >
> > Add VectorUtils.cpp -> llvm::propagateMetadata() to the list; that's
> used by
> > the Loop and SLP vectorizers. I've never looked at this closely (I'm
> here in
> > the context of PR28430), so I'm not sure what, if any, differences these
> > callers expect. It's also seems like combineMetadataForCSE is not
> behaving
> > ideally. For example, I got here because:
> >
> > define i8* @isa_impl_wrap(i8** %x) {
> >   %t2 = alloca i8*
> >   %t4 = load i8*, i8** %x
> >   store i8* %t4, i8** %t2
> >   %t6 = load i8*, i8** %t2, !nonnull !0   <--- we should preserve that to
> > optimize dyn_cast
> >   ret i8* %t6
> > }
> > !0 = !{}
> >
> > $ ./opt -instcombine cse_nonnull.ll -S
> > ...
> > define i8* @isa_impl_wrap(i8** %x) {
> >   %t4 = load i8*, i8** %x, align 8   <--- but it's gone now
> >   ret i8* %t4
> > }
> >
>
> Isn't this because we're combining %t4 and %t6 and we can set
> `!nonnull` if both instructions have the !nonnull metadata?
>
> From `llvm::combineMetadata()`:
>       case LLVMContext::MD_nonnull:
>         // Only set the !nonnull if it is present in both instructions.
>         K->setMetadata(Kind, JMD);
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170103/183937ee/attachment-0001.html>