[IR] Optimize mayBeDerefined for known linkages. NFC

[MaskRay]

Summary

For weak/linkonce/common/extern_weak (they are also interposable by default), just return true.
For internal/private/appending, there cannot be different copies, just return false.
Only for external, we need to call isInterposable: if it is dso_local, the
definition resolves to a symbol within the same linkage, and we can consider its
definition as exact; otherwise the definition is not exact.

[dblaikie]

Is this functionally changing behavior? (looks like it is, because the 'false' return could currently be overriden by the SemanticInterposable module metadata, resulting in a 'true' return even for these linkages?) If it is a functional change it should have test coverage at least.

(though I think it's unfortunate that this would make the noipa work more involved & add more slices of behavior that could be incorrectly used)

[MaskRay]

In D101264#2715553, @dblaikie wrote:

Is this functionally changing behavior? (looks like it is, because the 'false' return could currently be overriden by the SemanticInterposable module metadata, resulting in a 'true' return even for these linkages?) If it is a functional change it should have test coverage at least.

No. InternalLinkage and PrivateLinkage imply dso_local. dso_preemptable AppendingLinkage doesn't make sense.

(though I think it's unfortunate that this would make the noipa work more involved & add more slices of behavior that could be incorrectly used)

The noipa work needs a helper, not reusing the existing isDefinitionExact.

[dblaikie]

In D101264#2715563, @MaskRay wrote:

In D101264#2715553, @dblaikie wrote:

Is this functionally changing behavior? (looks like it is, because the 'false' return could currently be overriden by the SemanticInterposable module metadata, resulting in a 'true' return even for these linkages?) If it is a functional change it should have test coverage at least.

No. InternalLinkage and PrivateLinkage imply dso_local. dso_preemptable AppendingLinkage doesn't make sense.

Ah, right - I see in isInterposable. Makes sense.

Is this patch valuable, though - it duplicates some logic from isInterposable which could then become divergent on subsequent code changes? Duplicating logic is generally undesirable for that reason.

(though I think it's unfortunate that this would make the noipa work more involved & add more slices of behavior that could be incorrectly used)

The noipa work needs a helper, not reusing the existing isDefinitionExact.

I fairly strongly disagree with this - because I think going that way is likely to cause noipa to not be correctly supported in the future when someone adds another optimization, etc, and maybe fixes the interposable case but misses optnone or other cases.

I'm happy renaming those functions - and if there are users that really do need "isInterposable" or "isDefinitionExact" as distinct from this renamed function that includes optnone, I'd like to see those/discuss them.

[MaskRay]

In D101264#2717518, @dblaikie wrote:

In D101264#2715563, @MaskRay wrote:

In D101264#2715553, @dblaikie wrote:

Is this functionally changing behavior? (looks like it is, because the 'false' return could currently be overriden by the SemanticInterposable module metadata, resulting in a 'true' return even for these linkages?) If it is a functional change it should have test coverage at least.

No. InternalLinkage and PrivateLinkage imply dso_local. dso_preemptable AppendingLinkage doesn't make sense.

Ah, right - I see in isInterposable. Makes sense.

Is this patch valuable, though - it duplicates some logic from isInterposable which could then become divergent on subsequent code changes? Duplicating logic is generally undesirable for that reason.

I don't think isInterposable should change behavior due to noipa. The IPO amenable property uses the interposability information, not the other way around.
I could change some cases to return isInterposableLinkage(Linkage); instead of return true/return false, but I don't see the value sharing code this way.
To prevent unintentional changes, placing the two switches together may be one way, or make the comment (e.g. The above three cannot be overridden but can be de-refined.) clearer.

(though I think it's unfortunate that this would make the noipa work more involved & add more slices of behavior that could be incorrectly used)

The noipa work needs a helper, not reusing the existing isDefinitionExact.

I fairly strongly disagree with this - because I think going that way is likely to cause noipa to not be correctly supported in the future when someone adds another optimization, etc, and maybe fixes the interposable case but misses optnone or other cases.

I actually think the name isDefinitionExact is quite good. Adding noipa will make the name a misnomer, so we shouldn't reuse the name.

I'm happy renaming those functions - and if there are users that really do need "isInterposable" or "isDefinitionExact" as distinct from this renamed function that includes optnone, I'd like to see those/discuss them.

I currently disagree with a wholesale renaming. The use cases need analysis. We cannot assume every one wants overloaded noipa semantics.

[dblaikie]

In D101264#2718367, @MaskRay wrote:

In D101264#2717518, @dblaikie wrote:

In D101264#2715563, @MaskRay wrote:

In D101264#2715553, @dblaikie wrote:

Is this functionally changing behavior? (looks like it is, because the 'false' return could currently be overriden by the SemanticInterposable module metadata, resulting in a 'true' return even for these linkages?) If it is a functional change it should have test coverage at least.

No. InternalLinkage and PrivateLinkage imply dso_local. dso_preemptable AppendingLinkage doesn't make sense.

Ah, right - I see in isInterposable. Makes sense.

Is this patch valuable, though - it duplicates some logic from isInterposable which could then become divergent on subsequent code changes? Duplicating logic is generally undesirable for that reason.

I don't think isInterposable should change behavior due to noipa.

OK - that can be discussed in the noipa review thread, though.

I could change some cases to return isInterposableLinkage(Linkage); instead of return true/return false, but I don't see the value sharing code this way.
To prevent unintentional changes, placing the two switches together may be one way, or make the comment (e.g. The above three cannot be overridden but can be de-refined.) clearer.

Sorry, I'm not following - I'm trying to understand the value of this patch compared to the way it's currently written. Not duplicating logic seems like generally better/more maintainable code.

I'm not sure which switches you're suggesting to put together, though.

To me it seems like "mayBeDerefined" can reasonably make more things derefinable (as it does for the ODR linkages), but shouldn't make fewer things (ie: shouldn't ever hardcode "return false") compared to "isInterposable" - it should let any true result from isInterposable come through as it does today.

(though I think it's unfortunate that this would make the noipa work more involved & add more slices of behavior that could be incorrectly used)

The noipa work needs a helper, not reusing the existing isDefinitionExact.

I fairly strongly disagree with this - because I think going that way is likely to cause noipa to not be correctly supported in the future when someone adds another optimization, etc, and maybe fixes the interposable case but misses optnone or other cases.

I actually think the name isDefinitionExact is quite good. Adding noipa will make the name a misnomer, so we shouldn't reuse the name.

I'm happy renaming those functions - and if there are users that really do need "isInterposable" or "isDefinitionExact" as distinct from this renamed function that includes optnone, I'd like to see those/discuss them.

I currently disagree with a wholesale renaming. The use cases need analysis. We cannot assume every one wants overloaded noipa semantics.

If you know of any cases that's handling interposable in a way that wouldn't be also the right thing to do for noipa, it'd be really useful to have that data in the noipa review thread.

[jdoerfert]

Doesn't this change internal/appending/private dso_local with "semantic interposition" enabled? TBH, it might just be that those do not make sense (though we seem not to assert on them).
I can see the value of avoiding the roundtrip through isInterposable but the argument by @dblaikie that we should not duplicate stuff is valid too. Though, we already duplicate some stuff.
Maybe, as a compromise to "clean it up", we make a compile time constant array of linkage information

struct LinkageInformation {
  llvm::Optional<bool> MayBeDerefinedReturn;
  ...
};
LinakageInformation LIs[LinkageTypes::__LAST] = {
  LinkageInformation{llvm::None, ...}, // ExternalLinkage
  ...
  LinkageInformation{true, ...}, // WeakODRLinkage
};

bool mayBeDerefined() const {
  if (LIs[getLinkage()].MayBeDerefinedReturn) return *LIs[getLinkage()].MayBeDerefinedReturn;
  return isInterposable();
}

llvm/include/llvm/IR/GlobalValue.h

private:
   void destroyConstantImpl();
   Value *handleOperandChangeImpl(Value *From, Value *To);
 
   /// Returns true if the definition of this global may be replaced by a
   /// differently optimized variant of the same source level function at link
   /// time.
   bool mayBeDerefined() const {
     switch (getLinkage()) {
+    case WeakAnyLinkage:
+    case LinkOnceAnyLinkage:
+    case CommonLinkage:
+    case ExternalWeakLinkage:
+
+    // The three cannot be overridden but can be de-refined.
     case WeakODRLinkage:
     case LinkOnceODRLinkage:
     case AvailableExternallyLinkage:
       return true;
 
-    case WeakAnyLinkage:
-    case LinkOnceAnyLinkage:
-    case CommonLinkage:
-    case ExternalWeakLinkage:
-    case ExternalLinkage:
-    case AppendingLinkage:
     case InternalLinkage:
     case PrivateLinkage:
+    case AppendingLinkage:
+      return false;
+
+    case ExternalLinkage:
       return isInterposable();
     }
 
     llvm_unreachable("Fully covered switch above!");
   }
 
 protected:
   /// The intrinsic ID for this subclass (which must be a Function).