[PATCH] D101264: [IR] Optimize mayBeDerefined for known linkages. NFC
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 26 11:49:14 PDT 2021
dblaikie added a comment.
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.
>> (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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101264/new/
https://reviews.llvm.org/D101264
More information about the llvm-commits
mailing list