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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D101264#2718367 <https://reviews.llvm.org/D101264#2718367>, @MaskRay wrote:

> 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101264