Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi Andrea,
>
> On Wed, Jul 22, 2020 at 12:02:33PM +0200, Andrea Corallo wrote:
>> This first patch implements the addition of a new RTX instruction class
>> FILLER_INSN, which has been white listed to allow placement of NOPs
>> outside of a basic block. This is to allow padding after unconditional
>> branches. This is favorable so that any performance gained from
>> diluting branches is not paid straight back via excessive eating of
>> nops.
>
>> It was deemed that a new RTX class was less invasive than modifying
>> behavior in regards to standard UNSPEC nops.
>
> So I wonder if this cannot be done with some kind of NOTE, instead?
>
>> + /* Allow nops after branches, via FILLER_INSN. */
>> + bool fail = true;
>> + subrtx_iterator::array_type array;
>> + FOR_EACH_SUBRTX (iter, array, x, ALL)
>> + {
>> + const_rtx rtx = *iter;
>> + if (GET_CODE (rtx) == FILLER_INSN)
>> + {
>> + fail = false;
>> + break;
>> + }
>> + }
>> + if (fail)
>> + fatal_insn ("insn outside basic block", x);
>
> This stops checking after finding a FILLER_INSN as first insn. Is that
> on purpose?
I guess after one is expected to find only other fillers but yeah, I
missed this while reviving the patch, it should be improved agree.
>> +/* Make an insn of code FILLER_INSN to
>> + pad out the instruction stream.
>> + PATTERN should be from gen_filler_insn ().
>> + AFTER will typically be an unconditional
>> + branch at the end of a basic block. */
>
> Because it only allows one particular insn pattern, it should be pretty
> easy to use a NOTE for this, or even a normal INSN where the ouside-of-BB
> test can then just look at its pattern.
>
>
> As you see, I really do not like to have another RTX class, without very
> well defined semantics even. Not without first being shown no
> alternatives are acceptable, anyway :-)
I see I see :) I really don't have any strong opinion about this. I'll
be happy to rework the patch in this direction if this is the outcome
suggested by the code review.
Thanks!
Andrea
