In D112016#3071407, @rnk wrote:

In D112016#3070842, @aeubanks wrote:

happy to change the name if there's a better alternative

To bikeshed a bit, how about "load_eq"? Meaning, "a load of op1 is equal to op2".

I implemented this properly with Arthur in https://reviews.llvm.org/D109134

In D109548#2997429, @jdoerfert wrote:

volatile reads are considered write effects, IIRC: https://godbolt.org/z/73Tqjj4rc

In D109548#2996145, @nikic wrote:

In D109548#2995597, @aeubanks wrote:

In D109548#2995551, @jdoerfert wrote:

In D109548#2995532, @aeubanks wrote:

In D109548#2994483, @nikic wrote:

@Prazek writeonly only says that no memory is read, it does not determine which location is actually written. launder.invariant.groupdoes not read or write the pointer operands (it is inaccessiblememonly), it "writes" some inaccessible location to avoid code motion optimizations. To perform DSE, we need to know the location being written.

If a writeonly function is called multiple times, can it can only write a constant to a constant location or the argument? To be able to write to different locations across calls or write a different value, it'd have to do read memory to be able to determine when to do something different right? e.g. a global counter to write a different on multiple invocations.
So could we prove that it does the exact same thing across multiple back-to-back invocations given the same argument? Or could we consider this intrinsic to not really read to memory but still not produce the same results upon back-to-back invocations with the same argument?

"write a constant to a constant location" is only true if you have no arguments. Basically, constants + arguments is what you have to decide what and where to write. Same arguments + write only => same effect.

[EDIT] On second though. We might have intrinsics that make this more complicated as they allow you to get input without reading things. I'm not 100% sure but the "read thread ID" intrinsics on GPUs might fall into that category.

Basically can we optimize

%b = @llvm.writeonly(%a)
%c = @llvm.writeonly(%a)

to just

%b = @llvm.writeonly(%a)

and RAUW %c with %b

I'd argue that we can't without further information -- the write may be volatile.

Actually, as Krzysztof Pszeniczny pointed out offline, I think we can't mark it as writeonly, because it would let optimizer to optimize:

In D109548#2993926, @nikic wrote:

Do you have any examples of where this is relevant in practice?

The change looks sound to me, but should probably apply to all intrinsics that exist to model control dependence, i.e. assume, noalias.scope.decl, annotation etc.

This seems to be valid!
Can any func attr expert can comment on what sort of optimizations/reordering this enables? I am a bit worried we might be missing something.

Great! We will need to take a look at what NewGVN missed and understand if this is NewGVN fault, or are we missing something on MSSA site.

LGTM!

This looks resonable. BTW are {launder,strip}.inariant.group intrinsics considered to be free now? I remember fixing this in InlineCost, but this seems to be other cost heuristic. They totally should be considered free.

Could you also update the description and title?

LGTM! Great work with looking for regressions like that.

Just for the record (discussed offline): if the problem is comparisons of launder/strip(%p) with null, then for this specific case we could I think replace it with comparison of %p and null, as null does does not have any "virtual" information (similarly how we already optimzie launder/strip(null) to null

Lets talk offline about the specific case to get it right. The model is pretty brittle in those areas, so we have to be sure the transformations are legal

Sorry for late reply. Although I don't have experience in this pass, it looks good!

I had some time to look into it now, sorry that I missed it on first revision.
If I understand the LICM code correctly, it needs to drop all the instruction metadata when hoisting instruction that will not be executed unconditionally. This means that code like:
https://godbolt.org/z/bjP64PbhY

In D101103#2720276, @aeubanks wrote:

In D101103#2718965, @Prazek wrote:

Out of curiosity: have you seen this pattern happening in the wild with -fstrict-vtable-pointers?

Yes, it was causing a major size regression in one specific file (with rtti). I've managed to reduce it down to

struct A {
  int i;
  virtual void f();
  A(): i(4) {}
};

const A g;

int foo() {
  const A* a = &g;
  return a->i;
}

$ ./build/rel/bin/clang++ /tmp/a.cc -o - -S -emit-llvm -O2 -fstrict-vtable-pointers

%struct.A = type <{ i32 (...)**, i32, [4 x i8] }>

@_ZL1g = internal constant %struct.A <{ i32 (...)** bitcast (i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* @_ZTV1A, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 4, [4 x i8] zeroinitializer }>, align 8
@_ZTV1A = external dso_local unnamed_addr constant { [3 x i8*] }, align 8
@llvm.global_ctors = appending global [0 x { i32, void ()*, i8* }] zeroinitializer

; Function Attrs: nofree nosync nounwind readnone uwtable willreturn mustprogress
define dso_local i32 @_Z8hahahahav() local_unnamed_addr #0 {
entry:
  %0 = tail call i8* @llvm.strip.invariant.group.p0i8(i8* bitcast (%struct.A* @_ZL1g to i8*))
  %i = getelementptr inbounds i8, i8* %0, i64 8
  %1 = bitcast i8* %i to i32*
  %2 = load i32, i32* %1, align 8, !tbaa !6
  ret i32 %2
}

$ ./build/rel/bin/clang++ /tmp/a.cc -o - -S -emit-llvm -O2

; Function Attrs: nofree norecurse nosync nounwind readnone uwtable willreturn mustprogress
define dso_local i32 @_Z8hahahahav() local_unnamed_addr #0 {
entry:
  ret i32 4
}

In D101103#2721429, @aeubanks wrote:

looks like the strip is coming from CodeGenFunction::EmitLValueForField():

if (auto *ClassDef = dyn_cast<CXXRecordDecl>(rec)) {
  if (CGM.getCodeGenOpts().StrictVTablePointers &&
      ClassDef->isDynamicClass()) {
    // Getting to any field of dynamic object requires stripping dynamic
    // information provided by invariant.group.  This is because accessing
    // fields may leak the real address of dynamic object, which could result
    // in miscompilation when leaked pointer would be compared.
    auto *stripped = Builder.CreateStripInvariantGroup(addr.getPointer());
    addr = Address(stripped, addr.getAlignment());
  }
}

is this actually true?

Out of curiosity: have you seen this pattern happening in the wild with -fstrict-vtable-pointers?

In D101103#2718848, @lebedev.ri wrote:

I think the recursion is unfortunate there. How about something like

static Constant *ConstructLoadOperandConstant(Value *Op) {
  SmallVector<Value*, 16> Worklist;
  
  OpStack.emplace_back(Op);
  for(int I = 0; I != OpStack.size(); ++I) {
    Value* Op = OpStack.back();
    
    if (isa<Constant>(Op))
      break; // YAY!

    if (auto *BC = dyn_cast<BitCastOperator>(Op))
      OpStack.emplace_back(BC->getOperand(0)); // Recurse.

    if (auto *GEP = dyn_cast<GEPOperator>(Op)) {
      if (all_of(ArrayRef<Value*>(GEP->operands()).drop_front(),
          [](Value*Op) { return isa<Constant>(Op); }))
        OpStack.emplace_back(GEP->getOperand(0)); // Recurse.
    }

    if (auto *II = dyn_cast<IntrinsicInst>(Op)) {
      if (II->getIntrinsicID() == Intrinsic::strip_invariant_group ||
          II->getIntrinsicID() == Intrinsic::launder_invariant_group)
        OpStack.emplace_back(II->getOperand(0)); // Recurse.
    }
    
    return nullptr; // Can't look past this instruction. Give up.
  }
  
  // Alright! Reconstruct this chain as constant expressions.
  Constant* NewOp = cast<Constant>(Worklist.pop_back_val());
  while(!OpStack.empty()) {
    Value* Op = OpStack.pop_back_val();

    if (isa<BitCastOperator>(Op))
      NewOp = ConstantExpr::getBitCast(NewOp, Op->getType());

    if (auto *GEP = dyn_cast<GEPOperator>(Op)) {
      SmallVector<Constant *> Idxs;
      Idxs.reserve(GEP->getNumOperands() - 1);
      for (unsigned I = 1, E = GEP->getNumOperands(); I != E; ++I)
        Idxs.push_back(cast<Constant>(GEP->getOperand(I));
      return ConstantExpr::getGetElementPtr(GEP->getSourceElementType(), NewOp,
                                            Idxs, GEP->isInBounds(),
                                            GEP->getInRangeIndex());
    }

    if (auto *II = dyn_cast<IntrinsicInst>(Op)) {
      if (II->getIntrinsicID() == Intrinsic::strip_invariant_group ||
          II->getIntrinsicID() == Intrinsic::launder_invariant_group)
        NewOp = ConstantExpr::getBitCast(NewOp, Op->getType());  
    }
    
    llvm_unreachable("Instruction handled in first loop but not here?");
  }
}
  
return NewOp;

+1, strip.invariant.group probably needs the same handling. Thanks for fixing!

In D45151#1200094, @hfinkel wrote:

In D45151#1199948, @sanjoy wrote:

I'm sorry I can't be more decisive here since I wasn't deeply involved with the devirtualization work early on and so lack a lot of context. The general direction here seems fine to me -- given that we use metadata to express a large range of things, it seems ok to have metadata specific MD dropping policies. However, given that Hal originally objected to this, we should make sure he is on board.

I understand the motivation, thanks. The problem here is that, while we might want to avoid hoisting so that we don't strip the metadata, that only is better if we happen to inline into a function that then provides a concrete type. Otherwise, we should have hoisted. How about this: Add a parameter to LICM to control this choice: We can choose not to hoist during the early runs on LICM (which happen during inlining), and then hoist later (during the LICM invocation that runs after loop unrolling).

LGTM, but please wait for Javed to review

In D45151#1197581, @sanjoy wrote:

Can we spec !invariant.group in a way that lets us always keep the metadata when hoisting? Right now it isn't clear what happens if its contract is violated, i.e. what the behavior of this program is:

store i32 0, i32* %ptr, !invariant.group !0
%x = load i32, i32* %ptr, !invariant.group !0
store i32 1, i32* %ptr, !invariant.group !0
%y = load i32, i32* %ptr, !invariant.group !0

though it seems like you're assuming the *load* has UB? Can we instead say that the second store has UB? That way we should be able to hoist the load instruction without dropping the metadata.

friendly ping

fixed addrspace cast

Refactor

• Fixed comments

@hfinkel is my response resonable?

Friendly ping @hfinkel @rsmith

• Fixed templates

updated test

unreachable

@rjmccall @rsmith friendly ping

friendly ping

s/loop unrolling/loop peeling/
I will had to take a look at this at some time, but from what I've heard the state of loop peeling in LLVM is not very sofisticated, and implementing the logic that I propose might be a little hard.

In D45151#1113727, @hfinkel wrote:

You say that you don't yet know if this is profitable yet. Do you have reason to believe that it might not be profitable (e.g., some example where it seems like we might want to constrain the behavior)? We almost never favor keeping metadata over doing other transformations, so I think it's worth being explicit about our reasoning here. Just saying "I don't know yet" is probably not sufficient, as we could say that about nearly all metadata, and in that case, change the default logic.

In D47214#1109007, @xbolva00 wrote:

And two facts..

1. LLVM already replaces bzero with memset
2. There are many source codes related to networking which uses bcopy, bcmp.

So I dont think this patch is totally useless.

Maybe @Prazek can do review of this patch for me?

fixed test

small update

Fixed missing vtable commponents

fixed nit

one more test

Can someone with experience in Loop passes take a look at this?

OK, so it seems that at least the tests in the second patch passes without this patch, so I am fine with committing it without this one for one (also Philip if you could take a look at that patch I would be grateful)
I am worried about cases, where the first instruction does not throw, but it we can't hoist it (e.g. it depends on the loop phi) and after that, there is an instruction that can be hoisted.

Slitted commit into defining and using intrinsic

In D47108#1109014, @rjmccall wrote:

I thought we already had places in Sema that marked inline virtual methods as used, instantiated templates, etc. for devirtualization purposes when optimization was enabled. Did we rip that out?

I only recall the emitting available_externally vtables opportunistically, that is emitting it only if all the inline virtual functions are present (and they are not hidden).
(https://reviews.llvm.org/D33437)

• Add more comments

@rjmccall do you have any thoughts on the best way to solve it?

rebase

fix name

After rebasing

• small refactor

Changed comments

introduced mayBeDynamicClass and added more tests

Fixed flag passing

Fixed comment