llvm dev - Aug 2020 - 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 specified
consistently 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>

On Mon, Aug 31, 2020 at 4:50 PM Thomas Lively <tlively at google.com>
wrote:
> 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 specified
> consistently 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?
>
>
+Alina Sbirlea <asbirlea at google.com>

There are different hook points in the pipelines - I don't see a problem
with adding another one if we need to. :)

-eric

>
>
>
> 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/88877142/attachment.html>

On Mon, Aug 31, 2020 at 1:50 PM Thomas Lively via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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 specified
> consistently 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.
>
So if that's already the case, what would change between that state and
what I'm suggesting?

> 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
>>>
>> _______________________________________________
> 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/67e994b7/attachment-0001.html>

The difference is illustrated in this bug report:
https://github.com/rust-lang/rust/issues/74320. Essentially, frontends like
Rust want to allow users to opt-in to a feature by annotating a function
that will use it. Your suggestion to require that all functions have the
same feature set would prevent frontends from doing this sort of thing when
targeting WebAsssembly. Which on one hand would be entirely reasonable,
since annotating a single function with a feature misrepresents how
WebAssembly features work, but on the other hand would make WebAssembly
different from other targets, which I'd like to avoid.

On Mon, Aug 31, 2020 at 2:13 PM David Blaikie <dblaikie at gmail.com>
wrote:
>
>
> On Mon, Aug 31, 2020 at 1:50 PM Thomas Lively via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> 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 specified
>> consistently 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.
>>
>
> So if that's already the case, what would change between that state and
> what I'm suggesting?
>
>
>> 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
>>>>
>>> _______________________________________________
>> 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/6ebf134f/attachment-0001.html>