[PATCH] D101264: [IR] Optimize mayBeDerefined for known linkages. NFC

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D101264#2717518 <https://reviews.llvm.org/D101264#2717518>, @dblaikie wrote:

> In D101264#2715563 <https://reviews.llvm.org/D101264#2715563>, @MaskRay wrote:
>
>> In D101264#2715553 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101264/new/

https://reviews.llvm.org/D101264