Hi Jeff, 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). If you now use this in other places than combine, we will *never* be able to fix it. It's also shoddy architecture, of course. > So we don't have to duplicate any of that logic. I scanned > make_field_assignment and its children to make sure it didn't use any > data that was only valid during the combine pass, but I could have > missed something. This is one of those cases where encapsulating the > pass specific bits in a class really helps avoid problems... Just > saying.... It isn't the data that is the problem. combine has almost no private data. But the problem is all the hidden assumptions combine makes, and for example that it often make **non-canonical** RTL; this is no problem in combine, because that code will just not recog() (except of course that you then get less well optimised code, in general). (Often, combine makes that RTL canonical later, fwiw). 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. > Second, it relaxes the main insv pattern to allow an immediate in the > operand predicate, but forces it into a register during LRA. I don't > generally like having predicates that are looser than the constraints, > but it really helps here. Basically we want to see the constant field > we're going to insert. 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 "#"). > -static int > +int > get_pos_from_mask (unsigned HOST_WIDE_INT m, unsigned HOST_WIDE_INT *plen) If you want to reuse this, please give it a better name, and move it to rtlanal or some header file or similar? It may be nicer to make a more machine-specific routine for this, see rs6000_is_valid_mask for example. > +;; This must be split before register allocation and reloading as > +;; we need to verify it's actually an bitfield insertion by > +;; examining both immediate operands. After reload we will lose > +;; knowledge of operands[3] constant status. I don't understand what this means? After reload operands[3] is still a const_int? > +(define_insn_and_split "" Give this a name? Starting with * if you don't want a gen_* for it. > + [(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 :-( Segher
