Hans-Peter Nilsson writes:
> On Wed, 19 Aug 2020, Senthil Kumar Selvaraj wrote: >> >> Hans-Peter Nilsson writes: >> >> > On Fri, 14 Aug 2020, Senthil Kumar Selvaraj via Gcc wrote: >> >> As you can deduce from the (set_attr "cc" ..), only constraint >> >> alternatives 0,2,3 and 6 clobber CC - others leave it unchanged. >> > >> > Yes, I recognize that. >> > >> >> My first version of the port adds a post-reload splitter that adds a >> >> (clobber (reg:CC REG_CC)) unconditionally, and it appears to work. >> > >> > Ouch, temporarily lying to gcc here. >> > >> >> If I >> >> do want to add the clobber conditionally, would something like the below >> >> be a good way to do it (get_cc_reg_clobber_rtx returns either const0_rtx >> >> or cc_reg_rtx based on get_attr_cc (insn))? Or is there a better/cleaner >> >> way? >> > >> > I suggest having a look at what I did for the CRIS port. >> > Check the git history. >> > >> > In short: >> > - Add the clobber initially, to *all* patterns. >> > - Add postreload splitters for the special combinations that >> > don't clobber CC (ones without clobbering CC). >> > - Use the old "cc" attribute to control and generate >> > clobber/useful CC-setting alternatives (for various new CC_ >> > modes) with use of define_subst. >> >> This sounds like a great plan - the idea of always generating insn >> variants for however many CCmodes are available, and then using >> cc_enabled to disable them selectively (based on the attribute value >> corresponding to the alternative chosen) looks really neat. I did not >> understand the purpose of subst_attr for "ccnz" "ccnzvc" and "cccc" >> though - wouldn't a (set_attr "cc_enabled", "...") do the same thing? > > You mean adding a whole new line with separate per-alternative > settings instead of just a suffix to the name, per instruction, > to get it compare-elim-optimized? (Ok, plus a tweak to > SELECT_CC_MODE; see a33649e). That's not simpler to me, and > error-prone without benefits. Note that the "cc" attribute was > already there from cc0 times, just as for AVR! > > But beware, the cc attributes *might* need tweaking, and also > note that disabling an alternative for an insn may make gcc pick > a subsequent one, which may be invalid when matching operands > for the disabled alternative. > > But that's the simplistic and obvious reply, perhaps I'm > misinterpreting and you mean something else? For the purpose of > the different CCmodes, see cris-modes.def, in particular the > second paragraph, which I'm not repeating here. My question was a lot more basic. From the internals documentation, I understand that, with define_subst_attr for setcc, setnz and setnzc, and the corresponding define_substs, (define_insn "*movsi_internal<setcc><setnz><setnzvc> ...) results in a (define_insn "*movsi_internal ...) and in addition to that, (define_insn "*movsi_internal_setcc ...) // output template of setcc_subst (define_insn "*movsi_internal_setnz ...) // output template of setnz_subst (define_insn "*movsi_internal_setnzvc ...) // output template of setnzvc_subst I understood why this was done. What I didn't understand was the (set-attr "cc<cccc><ccnz><ccnzvc>") part - as far I can tell, this results in (set_attr "cc_enabled" ...) in all of the three substituted patterns, so I wondered why not just have (set_attr "cc_enabled" ...) in the original define_insn instead. I now realize that with (set-attr "cc<cccc><ccnz><ccnzvc>"), the original unsubstituted pattern will have only a (set_attr "cc" ...) and would therefore not match the attr check for "enabled" - correctly so, as the original insn pattern clobbers CRIS_CC0_REGNUM. Did I get that right? > >> Also, I already have a version that hides REG_CC until reload (based on >> the suggestion in the wiki page), but I guess this approach will work >> equally well with that too? > > Ow, I see it does! I don't know, that's unchartered territory > to me, but it does seem like visium was written that way. It > seems a bit wordy though; the define_split isn't necessary if > you have the clobber from the start and use define_subst for the > other alternatives. You still need splitters for special-cases > of insns where condition codes aren't affected though. If these > "special cases" approach being the norm, then I don't know. Ok, I'll try the cris approach too, and see if it makes any difference. > > IMHO, introducing REG_CC clobbers only *after* reload seems like > it could backfire. Doing things behind gcc's back is what we're > eliminating, not re-introducing, to be dogmatic. > > That wiki page mentions using define_subst to avoid all > (near-)duplicates, but not in its examples. Yes, I saw that too. Regards Senthil
