On 4/24/19 7:05 AM, Segher Boessenkool wrote: > 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). I think the structure of the pattern and its condition avoid the most problematic issues. But yea, it's not ideal.
>> 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. 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. > >> 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 "#"). 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. > >> -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? If we go forward, yea, makes sense. > > 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? It has to be an integer prior to register allocation. Otherwise we can't verify we have a valid field assignment. The pattern should never exist past the insn splitting pass. The comment is obsolete. It should always be split prior to register allocation into a field assignment. > >> +(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. > >> + [(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 :-) jeff
