[DeeployTest] Change order of typeMatching entries

[viv-eth]

This PR fixes a minor issue when inferring the input type. If the data range fits both the unsigned and signed version of a certain data type, it will pick first the signed version due to the current sorting mechanism. This is changed to prefer unsigned over signed datatypes.

Added

• Introduced a sorted_types list in the integer branch to capture integer types ordered by (typeWidth, isSigned).

Changed

• Replaced the old sorted(IntegerDataTypes, key=lambda x: x.typeWidth) loop with:

sorted_types = sorted(
    IntegerDataTypes,
    key=lambda t: (t.typeWidth, t.typeMin < 0),
)
for _type in sorted_types:
    …

Fixed

• Arrays with only non-negative values were previously inferred as signed if that happened to appear first; now they first try unsigned data types. This avoids errors when the input data sample is not "representative enough" to infer the type correctly.

PR Merge Checklist

1. The PR is rebased on the latest devel commit and pointing to devel.
2. Your PR reviewed and approved.
3. All checks are passing.
4. The CHANGELOG.md file has been updated.

[viv-eth]

After some investigation, I discovered that the following check fails:

    def typeCheckNodeInputs(self, ctxt: NetworkContext, node: gs.Node) -> bool:
        """DONT OVERRIDE - Type checks all input nodes to confirm they either already are assigned the correct type or their type can be statically upcast to the rule's input types

        Parameters
        ----------
        ctxt : NetworkContext
            Current NetworkContext
        node : gs.Node
            Node whose inputs should be analyzes

        Returns
        -------
        bool
            Whether the input's types match the rule's requirements

        """
        retCheck = True

        for inputNode, _type in zip(node.inputs, self.input_types):
            reference = ctxt.lookup(inputNode.name)

            if not isinstance(reference, VariableBuffer):
                return False

            if hasattr(reference, "values"):
                retCheck &= _type.referencedType.checkPromotion(reference.values)
            else:
                if ctxt.is_global(inputNode.name):
                    retCheck &= _type.referencedType.partialOrderUpcast(reference._type.referencedType)

                    if retCheck:
                        reference._type = _type
                        reference._instance = _type(inputNode.name, ctxt)
                else:
                    retCheck &= reference._type.referencedType == _type.referencedType
        return retCheck

Specifically, in this line:

retCheck &= reference._type.referencedType == _type.referencedType

, as reference._type.referencedType evaluates to <class 'Deeploy.CommonExtensions.DataTypes.uint8_t'> and _type.referencedType evaluates to <class 'Deeploy.CommonExtensions.DataTypes.int8_t'>. This means we run in the exact opposite case as before. Interestingly, it does not fail for the other platforms.

[viv-eth]

I fixed the failing test by generating input data for the Adder containing negative values, such that the test input is not in the range of uint8 anymore.

Warning: This is a lazy fix, however, could potentially lead to problems later. This is because the type inference inherently assumes some sort of type matching order.

Happy to discuss how to tackle this issue (potentially introducing some flags as hints for the compiler), but I think for now it's okay.

[Victor-Jung]

Does it make more sense to infer an unsigned first instead of a signed? There is no "good answer" if the input sample is biased (fundamentally, both answers are correct). Can you add an interface for the user to bypass the auto-inference system?

[Xeratec]

Does it make more sense to infer an unsigned first instead of a signed? There is no "good answer" if the input sample is biased (fundamentally, both answers are correct). Can you just add an interface for the user to bypass the auto-inference system?

Our assumption is that we can infer the minimum required type based on the input sample. For the bitwidth, this is obvious, and I argue it is more logical to infer an unsigned datatype if there are no negative numbers in the inputs. In summary, I agree with Vivi's changes, but I am also in favor of adding a flag to bypass the auto-inference.
(btw. If I remember correctly, this existed in the early days of Deeploy)

[Victor-Jung]

Does it make more sense to infer an unsigned first instead of a signed? There is no "good answer" if the input sample is biased (fundamentally, both answers are correct). Can you just add an interface for the user to bypass the auto-inference system?

Our assumption is that we can infer the minimum required type based on the input sample. For the bitwidth, this is obvious, and I argue it is more logical to infer an unsigned datatype if there are no negative numbers in the inputs. In summary, I agree with Vivi's changes, but I am also in favor of adding a flag to bypass the auto-inference.

(btw. If I remember correctly, this existed in the early days of Deeploy)

Okay I see the point of putting unsigned type first in the matching list. Let's get this flag setup and this PR rolling then @viv-eth 😁

[Xeratec]

@viv-eth Any plans to work on this in the next 2 weeks? Alternatively, I can offer to take it over, but will most likely include it in the next release.

[Xeratec]

Thanks for implementing the suggested changes. Please rebase the PR and address the remaining small comments and make sure all tests pass. Let me know if I should take over.

[Xeratec]

The changes were added under v0.2.0 in the Changelog and not under v0.2.1. I will change it, rebase it after merging #93, and merge it once the tests succeed. Thanks for your effort!

[Xeratec]

I would argue this is okay and ready for merge but please either @viv-eth or @lukamac give it a quick review.

[lukamac]

Besides the comments on the code, I have a question regarding the change of the Adder test. I saw in the beginning of the PR conversation that the change of the Adder test was a kind of stop-gap solution. Is it still necessary to change it? If not, I would revert it.

Also, I would mention in the changelog the new testTypeInferenceDifferentTypes and the deletion of the .gitlab-ci.yml.

[Xeratec]

Besides the comments on the code, I have a question regarding the change of the Adder test. I saw in the beginning of the PR conversation that the change of the Adder test was a kind of stop-gap solution. Is it still necessary to change it? If not, I would revert it.

Also, I would mention in the changelog the new testTypeInferenceDifferentTypes and the deletion of the .gitlab-ci.yml.

I’ve implemented all the requested changes, except for reverting the Adder test. The reason is that it still fails on the Snitch platform: the inputs are unsigned, but we currently don’t have an unsigned Add kernel in Snitch, nor do we support bias hoisting for the signed kernel.

[lukamac]

I’ve implemented all the requested changes, except for reverting the Adder test. The reason is that it still fails on the Snitch platform: the inputs are unsigned, but we currently don’t have an unsigned Add kernel in Snitch, nor do we support bias hoisting for the signed kernel.

Thanks for the effort :)

I'm thinking that it might make more sense to rename the current Adder test to AdderUnsigned and add this one as a new AdderSigned test and have the snitch platform only check the AdderSigned. I'm kinda reluctant to replace a test just because it fails on a specific platform because we kinda lose that information then.

[Xeratec]

I'm thinking that it might make more sense to rename the current Adder test to AdderUnsigned and add this one as a new AdderSigned test and have the snitch platform only check the AdderSigned. I'm kinda reluctant to replace a test just because it fails on a specific platform because we kinda lose that information then.

As discussed offline, we will keep the new test, which tests a signed addition.

[lukamac]

Approved