On Thu, 10 Aug 2023 19:12:06 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
On 5/29/23 06:46, Jeff Law wrote:
>
>
> On 5/29/23 05:01, Jin Ma wrote:
>> Reference:
>> https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5
>>
>> RISC-V should also be implemented to handle no_insn patterns for
>> pipelining.
>>
>> gcc/ChangeLog:
>>
>> * config/riscv/riscv.cc (riscv_sched_variable_issue): New function.
>> (TARGET_SCHED_VARIABLE_ISSUE): New macro.
>> ---
>> gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 3954fc07a8b..559fa9cd7e0 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -6225,6 +6225,24 @@ riscv_issue_rate (void)
>> return tune_param->issue_rate;
>> }
>> +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */
>> +
>> +static int
>> +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more)
>> +{
>> + if (DEBUG_INSN_P (insn))
>> + return more;
>> +
>> + rtx_code code = GET_CODE (PATTERN (insn));
>> + if (code == USE || code == CLOBBER)
>> + return more;
>> +
>> + if (get_attr_type (insn) == TYPE_UNKNOWN)
>> + return more;
>> +
>> + return more - 1;
>> +}
> The problem is that INSN is *much* more likely to be a real instruction
> that takes real resources, even if it is TYPE_UNKNOWN.
> TYPE_UNKNOWN here is actually an indicator of what I would consider a
> bug in the backend, specifically that we have INSNs that do not provide
> a mapping for the schedulers to suitable types.
>
> With that in mind I'd much rather get to the point where we can do
> something like this for TYPE_UNKNOWN:
>
> type = get_attr_type (insn);
> gcc_assert (type != TYPE_UNKNOWN);
>
> That way if we ever encounter a TYPE_UNKNOWN during development, we can
> fix it in the md files in a sensible manner. I don't know if we are
> close to being able to do that. We fixed a ton of stuff in bitmanip.md,
> but I don't think there's been a thorough review of the port to find
> other instances of TYPE_UNKNOWN INSNs.
Sorry for being lost here, but I'm not sure where TYPE_UNKNOWN comes
from. There's not a whole lot of instances in the code, and they all
seem to be doing something very special. Is it just something we didn't
do a '(set_attr "type" ...)' on?
In that case it seems reasonable to have a dev-mode early failure: we've
got some odd types now (like just the broad "bitmanip" one), but those
can be split later. At least having some classification seems like the
way to go, it's just an internal interface so we can make it better
later.
That said, it also smells like this is something that should be more
generic than backend code?
> The other thing if this code probably wants to handle GHOST type
> instructions. While GHOST is used for instructions which generate no
> code, it might seem they should return "more" as those INSNs take no
> resources. But GHOST is actually used for things like the blockage insn
> which should end a cycle from an issue standpoint. So the right
> handling of a GHOST is something like this:
>
> if (type == TYPE_GHOST)
> return 0;
Lost again, here, there's almost no references to TYPE_GHOST (aside from
a MIPS-ism that looks to have ended up in Loongarch).
So there wasn't ever any follow-up. Given this was something Ventana
was also carrying locally (with very minor differences) I went ahead and
merged up the implementations and pushed the final result to the trunk.
Attached is the patch that was actually committed.
Jeff
My fault, I'm very sorry for not replying to the patch follow-up, I just
forgot this :)
