Hello! 11.02.2020 16:40, Andrea Corallo wrote: > Hi Richard, > > "Richard Earnshaw (lists)" <richard.earns...@arm.com> writes: > >>> gcc/ChangeLog: >>> 2020-??-?? Andrea Corallo <andrea.cora...@arm.com> >>> 2020-??-?? Mihail-Calin Ionescu <mihail.ione...@arm.com> >>> 2020-??-?? Iain Apreotesei <iain.apreote...@arm.com> >>> * config/arm/arm.c (TARGET_INVALID_WITHIN_DOLOOP): >>> (arm_invalid_within_doloop): Implement invalid_within_doloop hook. >>> * config/arm/arm.h (TARGET_HAVE_LOB): Add new macro. >>> * config/arm/thumb2.md (*doloop_end, doloop_begin, dls_insn): >>> Add new patterns. >>> * config/arm/unspecs.md: Add new unspec. >>> >> A date should only appear before the first author in a multi-author >> patch, other authors should then be indented to align with the name of >> that first author. > Ack This patch is stage1 material, right? > >> +(define_insn "*doloop_end" >> + [(parallel [(set (pc) >> + (if_then_else >> + (ne (reg:SI LR_REGNUM) (const_int 1)) >> + (label_ref (match_operand 0 "" "")) >> + (pc))) >> + (set (reg:SI LR_REGNUM) >> + (plus:SI (reg:SI LR_REGNUM) (const_int -1)))])] >> + "TARGET_32BIT && TARGET_HAVE_LOB && !flag_modulo_sched" >> + "le\tlr, %l0") I'm not an expert in .md files, but having that "!flag_modulo_sched" condition seems wrong to me. What was the issue on SMS side to add that? Currently, there are fake doloop_end pattern on ARM. It is generated only when flag_modulo_sched is set and actually expands to more than one instruction. This old approach have its pros and cons. When we HAVE_LOB, target allows us to use a real doloop_end instruction, fake one is not needed at all. In this case compiler should use real instruction regardless whether SMS in on or off.
I hope in stage1 after upgrading modulo scheduler, we will restart old discussion about removing fake doloop_end pattern for ARM: https://gcc.gnu.org/ml/gcc-patches/2011-07/msg01812.html https://gcc.gnu.org/ml/gcc-patches/2012-01/msg00195.html Aarch64 also have such a fake pattern since 2014, probably its removal also will be considered. Roman >> Is it deliberate that this pattern name has a '*' prefix? doloop_end >> is a named expansion pattern according to md.texi. > Yes, this should be expanded already by the define_expand we have in > thumb2.md. Perhaps I'll call it 'doloop_end_internal' and add a > comment. > >> Also, hard-coded register names should be prefixed with '%|' (so >> "%|lr", not just "lr"), just in case the assembler dialect requires >> something (ELF doesn't but others have). Also for dls_insn. > Ack > >> For the tests, your 'require-effective-taret' tests look insufficient >> to prevent problems when testing a multilib environment, you'll need >> (at least) checks that a) passing -marm has not happened and b) that >> the architecture, or a specific CPU isn't being passed on the command >> line. > Ack > > Thanks for reviewing I'll update the patch. > > Andrea
