Page MenuHomePhabricator

[ValueTracking] Generalize isBytewiseValue into isSplatValue

Authored by bjope on Sep 14 2018, 6:03 AM.



Added a generalization of isBytewiseValue that is called
isSplatValue. It takes the requested splat size (in bits)
as an additional argument.

The old isBytewiseValue remains in the code as a shortcut
for isSplatValue(V, 8).

This refactoring can be useful both when looking for non
bytewise splats, but also for out-of-tree targets that
for example implement 16-bit bytes (as they now can
override the byte size used in isBytewiseValue more easily).

Diff Detail

Event Timeline

bjope created this revision.Sep 14 2018, 6:03 AM
bjope added subscribers: JesperAntonsson, uabelho.
jfb added a subscriber: jfb.Sep 20 2018, 11:34 AM
jfb added inline comments.Sep 20 2018, 11:36 AM

Your target has 16-bit bytes, shouldn't this function (with your change) call isSplatValue(V, 16) in your target?

bjope added inline comments.Sep 20 2018, 12:20 PM

Correct! We are actually doing return isSplatValue(V, DataLayout::getBitsPerByte()); in our llvm fork. But I can't do that in llvm trunk, because there is currently no way to ask for getBitsPerByte().

Currently we also need to implement/maintain our own version of isSplatValue, because the existing isBytewiseValue function in llvm trunk is not general enough.

jfb added inline comments.Sep 20 2018, 12:32 PM

Do you have plans to upstream your fork? It's otherwise pretty difficult to semi-support it, and it'll break very easily with changes that are useful to the LLVM community. It sounds like you're trying to incrementally get to a point where you can upstream?

bjope added inline comments.Sep 20 2018, 12:50 PM

I guess it will be hard to upstream full 16-bit bytes unless someone else is interested. There is not backend that supports it in tree, so it would be hard to create test cases.
Our target/backend is for special hardware so that will never go public.

This time I was just trying to reduce some of the diffs we have compared to upstream. The idea was that our more general isSplatValue instead of (or rather as a complement to) isBytewiseValue could be useful to the LLVM community as well.

But this is not uttermost important for us. It is just one out of many functions that we need to override to support 16-bit bytes. We are fully aware of the complexity with maintaining this in our fork. But we do not have that many alternatives. Rebasing to trunk everyday, and running lots of compiler tests keeps your mobile network happy! (that could be a new company slogan...)

jfb added inline comments.Sep 20 2018, 1:48 PM

I suggest starting a discussion on LLVM-dev. Changes such as this one don't improve LLVM as-is, and were I to see this code I'd simplify it by removing the indirection. If the LLVM community decides that it wants to support your type of effort then I'm all for incrementally making changes that'll get the support you want. If the community doesn't want to go in that direction (say, because nobody volunteers to do the work and maintain it), then I'm against this type of change because it adds burden without any eventual payoff. We'll just break you in the future it seems!

bjope added inline comments.Sep 21 2018, 1:24 AM

FWIW, I don't mind removing the indirection. I left it here as a backwards compatibility (in case isBytewiseValue was called from clang or etc).

We happened to have a more general isSplatValue implementation, so the goal was to contribute by replacing isBytewiseValue with our more general isSplatValue. After all, this is a common utility method, so having it specialized on 8-bit values seemed like a limitation. If this patch is a burden for LLVM community (although it did not add much complexity afaict) we can skip it.

jfb added inline comments.Sep 21 2018, 8:59 AM

As-is the indirection doesn't seem useful. It might be in a wider context, or if it's part of an incremental refactoring to do something the community wants.

What's more general about your implementation? If it's more general in that it also supports 16-bit bytes then I don't think we want to commit anything until the community decides this is a good idea. So I'm not saying "go away", I'm saying that a discussion should be had on llvm-dev about this.

Burdens are *fine* as long as they're worth it and are maintained.

bjope abandoned this revision.Sep 16 2019, 5:20 AM

Not pursuing this. Should have been abandoned a long time ago.

Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 5:20 AM