Thomas Lively via llvm-dev
2020-Aug-17 20:49 UTC
[llvm-dev] Inlining with different target features
Hi llvm-dev, I recently updated the WebAssembly TargetTransformInfo to allow functions with different target feature sets to be inlined into each other, but I ran into an issue I want to get the community's opinion on. Since WebAssembly modules have to be validated before they are run, it only makes sense to talk about WebAssembly features at module granularity rather than function granularity. The WebAssembly backend even runs a pass that applies the union of all used features to each function. That means that ideally inlining for the WebAssembly target would be able to disregard features entirely, since they will all be the same in the end. However, right now I have to be more conservative than that and only allow a callee to be inlined into a caller if the callee has a subset of the caller's features. Otherwise, a target intrinsic might end up being used in a function that does not have the necessary target features enabled, which would cause a validation failure. The best solution I can think of for this problem would be to allow targets to opt-in to having a caller's feature set updated to include the callee's feature set when the callee is inlined into the caller. This could be implemented via a new TTI hook, but a more general solution might be to change the return type of `areInlineCompatible` to allow targets to control this behavior on a case-by-case basis. Does this general direction sound ok, and if so, would it be better to add a new hook or add functionality to the existing one? Thanks, Thomas -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200817/447d2eab/attachment.html>
David Blaikie via llvm-dev
2020-Aug-31 16:51 UTC
[llvm-dev] Inlining with different target features
+echristo for thoughts on subtarget features and function inlining. Maybe target features aren't the right tool to model the WebAssembly situation? Perhaps you could model those with mergeable module-level metadata instead? Then the module would always have all the features and that would match the "we're allowed to union all features across all functions anyway" without it being a delayed pass that happens later. On Mon, Aug 17, 2020 at 1:49 PM Thomas Lively via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi llvm-dev, > > I recently updated the WebAssembly TargetTransformInfo to allow functions > with different target feature sets to be inlined into each other, but I ran > into an issue I want to get the community's opinion on. > > Since WebAssembly modules have to be validated before they are run, it > only makes sense to talk about WebAssembly features at module granularity > rather than function granularity. The WebAssembly backend even runs a pass > that applies the union of all used features to each function. That means > that ideally inlining for the WebAssembly target would be able to disregard > features entirely, since they will all be the same in the end. > > However, right now I have to be more conservative than that and only allow > a callee to be inlined into a caller if the callee has a subset of the > caller's features. Otherwise, a target intrinsic might end up being used in > a function that does not have the necessary target features enabled, which > would cause a validation failure. > > The best solution I can think of for this problem would be to allow > targets to opt-in to having a caller's feature set updated to include the > callee's feature set when the callee is inlined into the caller. This could > be implemented via a new TTI hook, but a more general solution might be to > change the return type of `areInlineCompatible` to allow targets to control > this behavior on a case-by-case basis. Does this general direction sound > ok, and if so, would it be better to add a new hook or add functionality to > the existing one? > > Thanks, > > Thomas > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200831/6f5894ee/attachment.html>
Thomas Lively via llvm-dev
2020-Aug-31 20:23 UTC
[llvm-dev] Inlining with different target features
Thanks for your reply! Using module metadata is an interesting idea, but it would require frontends to make wasm-specific changes to how they handle target features, which would be unfortunate. Working around that by extracting target features into metadata somewhere seems like it would be at least as intrusive as having the inliner update target features. We would also lose out on other valuable utilities like the ability to limit target intrinsics to certain features. On Mon, Aug 31, 2020 at 9:52 AM David Blaikie <dblaikie at gmail.com> wrote:> +echristo for thoughts on subtarget features and function inlining. > > Maybe target features aren't the right tool to model the WebAssembly > situation? Perhaps you could model those with mergeable module-level > metadata instead? Then the module would always have all the features and > that would match the "we're allowed to union all features across all > functions anyway" without it being a delayed pass that happens later. > > On Mon, Aug 17, 2020 at 1:49 PM Thomas Lively via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hi llvm-dev, >> >> I recently updated the WebAssembly TargetTransformInfo to allow functions >> with different target feature sets to be inlined into each other, but I ran >> into an issue I want to get the community's opinion on. >> >> Since WebAssembly modules have to be validated before they are run, it >> only makes sense to talk about WebAssembly features at module granularity >> rather than function granularity. The WebAssembly backend even runs a pass >> that applies the union of all used features to each function. That means >> that ideally inlining for the WebAssembly target would be able to disregard >> features entirely, since they will all be the same in the end. >> >> However, right now I have to be more conservative than that and only >> allow a callee to be inlined into a caller if the callee has a subset of >> the caller's features. Otherwise, a target intrinsic might end up being >> used in a function that does not have the necessary target features >> enabled, which would cause a validation failure. >> >> The best solution I can think of for this problem would be to allow >> targets to opt-in to having a caller's feature set updated to include the >> callee's feature set when the callee is inlined into the caller. This could >> be implemented via a new TTI hook, but a more general solution might be to >> change the return type of `areInlineCompatible` to allow targets to control >> this behavior on a case-by-case basis. Does this general direction sound >> ok, and if so, would it be better to add a new hook or add functionality to >> the existing one? >> >> Thanks, >> >> Thomas >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200831/a70a08f4/attachment.html>
Eric Christopher via llvm-dev
2020-Aug-31 20:39 UTC
[llvm-dev] Inlining with different target features
Hi Thomas, I'd prefer not to change areInlineCompatible because I think it reads fairly closely what is expected here (also see x86 for how this is used for subset inlining calculations). I think if you plan on updating all of the features for the functions to match you might just want to do that initially rather than try to update them piecemeal after or during inlining. Thoughts? -eric On Mon, Aug 17, 2020 at 4:49 PM Thomas Lively via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hi llvm-dev, > > I recently updated the WebAssembly TargetTransformInfo to allow functions > with different target feature sets to be inlined into each other, but I ran > into an issue I want to get the community's opinion on. > > Since WebAssembly modules have to be validated before they are run, it > only makes sense to talk about WebAssembly features at module granularity > rather than function granularity. The WebAssembly backend even runs a pass > that applies the union of all used features to each function. That means > that ideally inlining for the WebAssembly target would be able to disregard > features entirely, since they will all be the same in the end. > > However, right now I have to be more conservative than that and only allow > a callee to be inlined into a caller if the callee has a subset of the > caller's features. Otherwise, a target intrinsic might end up being used in > a function that does not have the necessary target features enabled, which > would cause a validation failure. > > The best solution I can think of for this problem would be to allow > targets to opt-in to having a caller's feature set updated to include the > callee's feature set when the callee is inlined into the caller. This could > be implemented via a new TTI hook, but a more general solution might be to > change the return type of `areInlineCompatible` to allow targets to control > this behavior on a case-by-case basis. Does this general direction sound > ok, and if so, would it be better to add a new hook or add functionality to > the existing one? > > Thanks, > > Thomas > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200831/b83de0fd/attachment.html>
Thomas Lively via llvm-dev
2020-Aug-31 20:50 UTC
[llvm-dev] Inlining with different target features
David, That's right, WebAssembly does not have a way to conditionally use a feature or even do runtime feature testing right now. It's on our roadmap of things to design and standardize, but it is still a long way off.> Another direction would be to require the features to be specifiedconsistently for all components of the build, I guess - if that's the net effect anyway. Would make portable libraries difficult, though - because they'd be linked into different things with different feature sets and that would violate the invariant. I agree this would be reasonable. We already require separate builds of a library for each feature set the library wants to support, so this wouldn't make that story any worse. The only reason I wouldn't want to enforce this is because that would be another way targeting Wasm would be different from targeting other platforms for frontends. Eric, Updating all the features up front would indeed make the most sense. I haven't seen a way for backends to specify LLVM IR passes that should be run early, though. Is that possible, or would frontends have to add this extra pass when targeting Wasm? On Mon, Aug 31, 2020 at 1:40 PM Eric Christopher <echristo at gmail.com> wrote:> Hi Thomas, > > I'd prefer not to change areInlineCompatible because I think it reads > fairly closely what is expected here (also see x86 for how this is used for > subset inlining calculations). I think if you plan on updating all of the > features for the functions to match you might just want to do that > initially rather than try to update them piecemeal after or during > inlining. > > Thoughts? > > -eric > > On Mon, Aug 17, 2020 at 4:49 PM Thomas Lively via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Hi llvm-dev, >> >> I recently updated the WebAssembly TargetTransformInfo to allow functions >> with different target feature sets to be inlined into each other, but I ran >> into an issue I want to get the community's opinion on. >> >> Since WebAssembly modules have to be validated before they are run, it >> only makes sense to talk about WebAssembly features at module granularity >> rather than function granularity. The WebAssembly backend even runs a pass >> that applies the union of all used features to each function. That means >> that ideally inlining for the WebAssembly target would be able to disregard >> features entirely, since they will all be the same in the end. >> >> However, right now I have to be more conservative than that and only >> allow a callee to be inlined into a caller if the callee has a subset of >> the caller's features. Otherwise, a target intrinsic might end up being >> used in a function that does not have the necessary target features >> enabled, which would cause a validation failure. >> >> The best solution I can think of for this problem would be to allow >> targets to opt-in to having a caller's feature set updated to include the >> callee's feature set when the callee is inlined into the caller. This could >> be implemented via a new TTI hook, but a more general solution might be to >> change the return type of `areInlineCompatible` to allow targets to control >> this behavior on a case-by-case basis. Does this general direction sound >> ok, and if so, would it be better to add a new hook or add functionality to >> the existing one? >> >> Thanks, >> >> Thomas >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200831/0cd2d741/attachment.html>