Re: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)

[Donal F. <don...@ma...>]

In general, this was on the verge of not being TIPped at all; it's only there really because of the impact on the TDK compiler/tbcload. Because of this, I'm pushing it through the process faster than normal.

Re performance being noise, that's about what I'd hope for. It's not a branch where performance is a primary focus (though some paths may be significantly better, especially around [try]).

If MSVC can't deprecate an enum element, we'll just make that a no-op. It's just a tweak to the conditions on how to define the macro. (It's formally a C++17/C23 feature when applied to an enumerator.)

Re the range checks, I guess we'll need to fix the type of unsigned operands (as part of the Tcl_Size project). The bytecode compiler is now matching the usage, but the usage in TEBC is itself wrong. (I shudder to think what code that makes a stack frame with that many local variable entries would look like, and it will certainly take quite a while to compile.) This particular branch isn't meant to touch LOCAL meaningfully, but LVT indices are definitely intended to be positive (with -1 being "no LVT entry") so the assumption that they're able to be handled as unsigned int within TEBC is likely a good one; the -1 should never result in an issue of opcodes that use LVT entries.

I wish we could use <stdint.h> (and <stdbool.h> as well).

________________________________
From: Ashok Nadkarni
Sent: Monday, May 12, 2025 14:59
To: Donal Fellows; Tcl Core
Subject: Re: TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)

Donal,

While I'm all for improvements in the byte code compiler and engine, even if only for clarity, I feel the TIP is so substantial as needing more time for people to digest.

Some preliminary performance tests (only based on the very limited tests in the perf-tests directory) indicate performance difference is in statistical noise, marginally faster in some cases, slower in others. Nevertheless, I would agree that simplification is desirable even for clarity/readability purposes.

A couple of implementation related comments...

Does not build with MS VC++ because the [[deprecated]] attribute is only available in C++14  onwards and not part of any C standard. Additionally, even for C++, VC++ does not support the attribute for enum values. Just making the defining macro a no-op fixes this at the loss of deprecation warnings in VC++. Not a big deal I feel.

The other question I have is with the numerous "value > INT_MAX" error checks during compilation being changed to "value > UINT_MAX". The "value" in question is generally Tcl_Size so on 64-bit platforms the change means that value in the range [INT_MAX+1, UINT_MAX] can now enter the bytecode stream. My impression of the byte code engine is that there are many implicit assumptions about integer value ranges and some byte codes can handle that range, and some cannot, despite the use of TclGetUInt4AtPtr implying unsigned values.

To work through the first incidence on a diff as an example, in TclCompCmds.c->TclCompileArrayExistsCmd, the fragment

    if (localIndex >= 0) {
      OP4(              ARRAY_EXISTS_IMM, localIndex);
    } else {
      OP(               ARRAY_EXISTS_STK);
    }

can now push the immediate value localIndex=0x80000000 onto the stack which it would not do before.

The INST_ARRAY_EXISTS_IMM case in TEBCResume does

      opnd = TclGetUInt4AtPtr(pc + 1);
      ...
      varPtr = LOCAL(opnd);

The macro LOCAL expands to

   #define LOCAL(i)     (&compiledLocals[(i)])

The problem I see is that opnd is typed as an "int", so 0x80000000 will be treated as a large negative index into compiledLocals with obvious consequences.

Perhaps this particular case could be fixed by changing LOCAL to cast i as unsigned but I seem to recall other similar cases as well when I last looked at the code during the signed/unsigned Tcl_Size debate and I'm personally not comfortable with such casts without a complete code review.

It is quite possible I have overlooked something but I do not think the INT_MAX->UINT_MAX changes are worth making unless there is certainty the TEBCExecute can handle unsigned ints in all cases and that is likely quite tricky and not worth the effort or risk imo.

Of course, values greater than INT_MAX will likely never occur in practice but in that case why make the change at all.

But again, I think folks (certainly me) need more time to look at it.

/Ashok
________________________________
From: Donal Fellows <don...@ma...>
Sent: Friday, May 9, 2025 5:46 PM
To: Tcl Core <tcl...@li...>
Subject: [TCLCORE] TIP 720: Updated Tcl Bytecode Opcodes CFV (NOTE: NO DELAY)

As promised, I've done a TIP for the updates to the bytecode compiler. It's largely there to document the notable features of a substantial changeset.

https://core.tcl-lang.org/tips/doc/trunk/tip/720.md [core.tcl-lang.org]<https://urldefense.com/v3/__https://core.tcl-lang.org/tips/doc/trunk/tip/720.md__;!!PDiH4ENfjr2_Jw!AZYcnp0vbPAsnExo4ZBiiFXx1UOLBYeTN1c8BqoZfUYc96p9LQop1KOylIPhICp-0gsz7c_3Xe76k5_1w3RPHKL9WELa1b7uZmXl$>

It covers the essentials; of particular note is that no true public API is affected. That said, tcl::unsupported::assemble is affected, but via enhancement (it gets new opcodes), and tdkcompiler/tbcload will definitely need more work due to the new auxiliary structure type (for a numeric jump table).

Given the lack of public surface changes - if you're not messing with compilation guts, you shouldn't need to care - I'm going to call a vote on this TIP immediately.

Votes to here by [clock format 1747393200] (Fri May 16 12:00:00 BST 2025) in the usual form.

TIP 720: YES

Donal.