On Wed, Apr 24, 2019 at 12:16:39PM -0600, Jeff Law wrote: > On 4/24/19 7:05 AM, Segher Boessenkool wrote: > > On Mon, Apr 22, 2019 at 09:09:12AM -0600, Jeff Law wrote: > >> First, it re-uses combine's make_field_assignment in the aarch64 > >> backend. > > > > That's not really acceptable. make_field_assignment depends intimately > > on make_extraction, which is very much combine-specific (and wrong in so > > many ways it's hard to tell). > I think the structure of the pattern and its condition avoid the most > problematic issues. But yea, it's not ideal.
Oh, I'm not saying it does not work, I trust you tested it well. But there are so many ways this can completely break (with ICEs and everything), this really isn't good. You say all the problematic cases cannot happen... So make a clone of this function without any of those cases? :-) > > But it _is_ a big problem in other contexts: you should not create > > non-canonical RTL. In your specific case, where you want to run this > > from a splitter, you have to make sure you get only correct, recognised > > patterns for your machine. make_field_extraction cannot guarantee you > > that. > Even in the case where the form and operands are limited to what's in > the new pattern? My worry with make_extraction is those cases where it > gets too smart for its own good and returns something even simpler than > extraction. I'm not sure that can happen in this case or not. > > I'm not keen on duplicating the logic from make_field_assignment and > make_extraction. Though again, given the limited forms we accept it may > not be too bad. Just praying that the RTL it creates is the RTL you prefer to see, or even just RTL the backend can handle, doesn't sound too great. (And any time this breaks it is a target bug, not combine). > > You also have an insn condition that can become invalid by the time the > > pattern is split (the !reload_complted part), which means the pattern > > will then *not* be split, which won't work here (the template is "#"). > The pattern is split by the splitting pass before register allocation > and reload. It's never supposed to exist beyond that splitting pass. > I'm pretty sure we've done this kind of thing regularly in the past. Are you sure this code can never be created between the split pass and when reload is finished? Like maybe during LRA itself. It's hard to convince myself of this. > >> +(define_insn_and_split "" > > > > Give this a name? Starting with * if you don't want a gen_* for it. > Didn't figure it was worth the trouble. But it's easy enough to do. It helps debugging (and it helps naming stuff in the changelog :-) ) > >> + [(set (match_operand:GPI 0 "register_operand" "=r") > >> + (ior:GPI (and:GPI (match_operand:GPI 1 "register_operand" "0") > >> + (match_operand:GPI 2 "const_int_operand" "n")) > >> + (match_operand:GPI 3 "const_int_operand" "n")))] > >> + "(!reload_completed > >> + /* make_field_assignment doesn't handle subregs well. */ > >> + && REG_P (operands[0]) > > > > Another reason to not use make_field_assignment, I'm afraid :-( > Perhaps. I'd probably want to avoid SUBREGs here regardless :-) You get a subreg for using a 64-bit pseudo as 32-bit (a pseudo has only one mode). This happens a lot for insert insns, from what I see. Segher
