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

[Davide Italiano via llvm-commits]

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