> In the experience I just had, it is quite error-prone to have multiple > tblgen patterns to match these things. The way things were before, > there was a clean separation between checking/enforcing node legality > and doing the final code selection, with isel being automatic through > tblgen. That was nice. The current setup mixes the two and seems to > result in more code in the form of additional tblgen patterns. We also > lose the ability to do shuffle peeps or any other such things unless we > teach the code about every type of special target node. > > It really doesn't seem worth it to me.In the way it was done before, every shuffle that we tried to match had to be checked twice (masks used to be checked during legalization and during isel by the tblgen patterns), this is done only once now (during legalization). Although we still match the node itself through tblgen patterns, the tablegen patterns are a lot more clear now, and we were able to remove lots of confusing code. The code used to be *very* fragile, during legalization there was no explicit rule or comments of what and when stuff needed to be matched (and changing a single rule would generate a code that would fail to match tblgen patterns later), all the logic was getting really hard to understand. The long term plan is: generate all target specific nodes during legalization, and once all logic is clear, we can go for more fancy stuff like having per-processor family shuffle tables describing the more profitable shuffles for a specific mask, and so on. The work stopped because of this bug: http://llvm.org/bugs/show_bug.cgi?id=8156, but IMHO the implementation of x86 shuffle matching is a lot more clear now then they used to be in the past. -- Bruno Cardoso Lopes http://www.brunocardoso.cc
Bruno Cardoso Lopes <bruno.cardoso at gmail.com> writes:>> It really doesn't seem worth it to me. > > In the way it was done before, every shuffle that we tried to match > had to be checked twice (masks used to be checked during legalization > and during isel by the tblgen patterns),Right.> this is done only once now (during legalization).Maybe. We still have the old operators like unpck and shup, so couldn't those still trigger? Shouldn't we remove them if we're using this TargetNode method? Is it very expensive to check masks, in the grand scheme of things?> Although we still match the node itself through tblgen patterns, the > tablegen patterns are a lot more clear now, and we were able to remove > lots of confusing code.That's more to do with the way the patterns grew organically than where the matching happens.> The code used to be *very* fragile, during legalization there was no > explicit rule or comments of what and when stuff needed to be matched > (and changing a single rule would generate a code that would fail to > match tblgen patterns later),Can you give an example? When I run into something like that, it's almost always because a useful pattern is missing.> all the logic was getting really hard to understand.Oh yes, it's messy stuff all right. But as I'm moving patches from 2.7 to trunk, I'm not noticing much improvement in the legalization code. Probably it's still a work in progress, yes?> The long term plan is: generate all target specific nodes during > legalization, and once all logic is clear, we can go for more fancy > stuff like having per-processor family shuffle tables describing the > more profitable shuffles for a specific mask, and so on. The work > stopped because of this bug: > http://llvm.org/bugs/show_bug.cgi?id=8156,Hmm...Again, isn't this due to the matching code being moved from isel to legalize? It seems like legalize is trying to do too much. It's not just "legalize" anymore, it's "legalize and optimize." When I added support for AVX shuffles (which I am upstreaming right now), I tried to do it in a systematic way. Maybe it will help to reorganize the other checks in a similar fashion. I haven't really thought about the bigger picture enough yet.> but IMHO the implementation of x86 shuffle matching is a lot more > clear now then they used to be in the past.There's certainly been improvement on the TableGen side of things. I really liked the unpck*, shufp, etc. nodes and the ShuffleVectorSDNode. That's a huge help. It's too bad we're getting rid of them. But legalization still looks about the same to me. Thanks for the explanation. -Dave
> Maybe. We still have the old operators like unpck and shup, so couldn't > those still trigger? Shouldn't we remove them if we're using this > TargetNode method? > > Is it very expensive to check masks, in the grand scheme of things?Probably not, in the old scheme the masks could be checked more than once during legalization and also more than once in the tablegen file (since the same mask is used in several patterns). But even if it's not a big improvement in efficiency IMO the "more readable" approach is still a big win.>> Although we still match the node itself through tblgen patterns, the >> tablegen patterns are a lot more clear now, and we were able to remove >> lots of confusing code. > > That's more to do with the way the patterns grew organically than where > the matching happens.Yes, but we have to start somewhere...>> The code used to be *very* fragile, during legalization there was no >> explicit rule or comments of what and when stuff needed to be matched >> (and changing a single rule would generate a code that would fail to >> match tblgen patterns later), > > Can you give an example? When I run into something like that, it's > almost always because a useful pattern is missing.I don't remember off hand, but I keep the memories of often running into different issues because of how fragile the code used to be.>> all the logic was getting really hard to understand. > > Oh yes, it's messy stuff all right. But as I'm moving patches from 2.7 > to trunk, I'm not noticing much improvement in the legalization code. > Probably it's still a work in progress, yes?Yes.>> The long term plan is: generate all target specific nodes during >> legalization, and once all logic is clear, we can go for more fancy >> stuff like having per-processor family shuffle tables describing the >> more profitable shuffles for a specific mask, and so on. The work >> stopped because of this bug: >> http://llvm.org/bugs/show_bug.cgi?id=8156, > > Hmm...Again, isn't this due to the matching code being moved from isel > to legalize? It seems like legalize is trying to do too much. It's not > just "legalize" anymore, it's "legalize and optimize." > > When I added support for AVX shuffles (which I am upstreaming right > now), I tried to do it in a systematic way. Maybe it will help to > reorganize the other checks in a similar fashion. I haven't really > thought about the bigger picture enough yet. > >> but IMHO the implementation of x86 shuffle matching is a lot more >> clear now then they used to be in the past. > > There's certainly been improvement on the TableGen side of things. I > really liked the unpck*, shufp, etc. nodes and the ShuffleVectorSDNode. > That's a huge help. It's too bad we're getting rid of them. But > legalization still looks about the same to me.The idea is to use tablegen again once we have a clean implementation. It would be good to have all tables and per-processor logic in a tablegen file, but I think a clean implementation needs to be done first. Anyway, I don't have time to work in any of this at the moment, and I'm not also the one with final word on this. I'm happy to help with the bugs that may appear and with some design ideas though.> Thanks for the explanation.You're welcome. -- Bruno Cardoso Lopes http://www.brunocardoso.cc