ronantakizawa/codereview-bench huggingface dataset:
Repository: dotnet/runtime
File: src/coreclr/jit/compiler.h (C/C++)

Code before:
        {
            return regMask;
        }

    private:
        regNumber reg;
        regMaskTP regMask;
    };

    VirtualStubParamInfo* virtualStubParamInfo = nullptr;

    bool IsTargetAbi(CORINFO_RUNTIME_ABI abi)
    {
        return eeGetEEInfo()->targetAbi == abi;
    }

    bool BlockNonDeterministicIntrinsics(bool mustExpand)
    {
        // We explicitly block these APIs from being expanded in R2R
        // since we know they are non-deterministic across hardware

        if (IsReadyToRun())
        {
            if (mustExpand)
            {
                implPrejitUnsupported();
            }
            return true;
        }
        return false;
    }

    bool generateCFIUnwindCodes()
    {
#if defined(FEATURE_CFI_SUPPORT)
        return TargetOS::IsUnix && IsTargetAbi(CORINFO_NATIVEAOT_ABI);
#else
        return false;
#endif
    }

    // Debugging support - Line number info

    void eeGetStmtOffsets();

    unsigned eeBoundariesCount;

    ICorDebugInfo::OffsetMapping* eeBoundaries; // Boundaries to report to the EE
    void                          eeSetLIcount(unsigned count);
    void eeSetLIinfo(unsigned which, UNATIVE_OFFSET offs, IPmappingDscKind kind, const ILLocation& loc);
    void eeSetLIdone();

Review comment (suggestion):
```suggestion
                implReadyToRunUnsupported();
```
We do not really use "prejit" when referring to R2R. Also, "prejit" is ambiguous since it is not clear whether it includes NAOT too.

Code after:
        {
            return regMask;
        }

    private:
        regNumber reg;
        regMaskTP regMask;
    };

    VirtualStubParamInfo* virtualStubParamInfo = nullptr;

    bool IsTargetAbi(CORINFO_RUNTIME_ABI abi)
    {
        return eeGetEEInfo()->targetAbi == abi;
    }

    bool BlockNonDeterministicIntrinsics(bool mustExpand)
    {
        // We explicitly block these APIs from being expanded in R2R
        // since we know they are non-deterministic across hardware

        if (IsReadyToRun())
        {
            if (mustExpand)
            {
                implReadyToRunUnsupported();
            }
            return true;
        }
        return false;
    }

    bool generateCFIUnwindCodes()
    {
#if defined(FEATURE_CFI_SUPPORT)
        return TargetOS::IsUnix && IsTargetAbi(CORINFO_NATIVEAOT_ABI);
#else
        return false;
#endif
    }

    // Debugging support - Line number info

    void eeGetStmtOffsets();

    unsigned eeBoundariesCount;

    ICorDebugInfo::OffsetMapping* eeBoundaries; // Boundaries to report to the EE
    void                          eeSetLIcount(unsigned count);
    void eeSetLIinfo(unsigned which, UNATIVE_OFFSET offs, IPmappingDscKind kind, const ILLocation& loc);
    void eeSetLIdone();
Repository: dotnet/runtime
File: src/coreclr/tools/Common/JitInterface/WasmLowering.cs (C#)

Code before:
            bool hasThis = false;
            bool explicitThis = false;

            if (loweredReturnType == null)
            {
                hasReturnBuffer = true;
                returnIsVoid = true;
            }
            else if (loweredReturnType.IsVoid)
            {
                returnIsVoid = true;
            }

            System.Collections.Generic.List<WasmValueType> result = new(signature.Length + 4);

            if (!signature.IsStatic)
            {
                hasThis = true;

                if (signature.IsExplicitThis)
                {
                    explicitThis = true;
                }
                else
                {
                    throw new NotImplementedException("Implicit this");
                }
            }

            if (method.IsUnmanagedCallersOnly) // reverse P/Invoke
            {
                if (hasReturnBuffer)
                {
                    result.Add(pointerType);
                }
            }
            else // managed call
            {
                result.Add(pointerType); // Stack pointer parameter

                if (hasThis)
                {
                    result.Add(pointerType);
                }

                if (hasReturnBuffer)
                {
                    result.Add(pointerType);
                }
            }


Review comment (suggestion):
I think you can delete this whole `else` clause.

We use `hasThis` to add the this pointer param, so there's nothing we need to do here for implicit this.


Code after:
            bool returnIsVoid = false;
            bool hasThis = false;
            bool explicitThis = false;

            if (loweredReturnType == null)
            {
                hasReturnBuffer = true;
                returnIsVoid = true;
            }
            else if (loweredReturnType.IsVoid)
            {
                returnIsVoid = true;
            }

            // Reserve space for potential implicit this, stack pointer parameter, portable entrypoint parameter, and return buffer
            ArrayBuilder<WasmValueType> result = new(signature.Length + 4);

            if (!signature.IsStatic)
            {
                hasThis = true;

                if (signature.IsExplicitThis)
                {
                    explicitThis = true;
                }
            }

            if (method.IsUnmanagedCallersOnly) // reverse P/Invoke
            {
                if (hasReturnBuffer)
                {
                    result.Add(pointerType);
                }
            }
            else // managed call
            {
                result.Add(pointerType); // Stack pointer parameter

                if (hasThis)
                {
                    result.Add(pointerType);
                }

                if (hasReturnBuffer)
                {
                    result.Add(pointerType);
                }
            }

            for (int i = explicitThis ? 1 : 0; i < signature.Length; i++)
            {
Repository: dotnet/runtime
File: src/coreclr/jit/codegenwasm.cpp (C++)

Code before:
            ins = INS_f64_gt;
            break;
        default:
            unreached();
    }

    GetEmitter()->emitIns(ins);

    if (invertSense)
    {
        GetEmitter()->emitIns(INS_i32_eqz);
    }

    WasmProduceReg(treeNode);
}

//------------------------------------------------------------------------
// genCodeForStoreBlk: Produce code for a GT_STORE_BLK node.
//
// Arguments:
//    tree - the node
//
void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp)
{
    assert(blkOp->OperIs(GT_STORE_BLK));
    assert(!blkOp->gtBlkOpGcUnsafe);

    bool isCopyBlk = blkOp->OperIsCopyBlkOp();

    switch (blkOp->gtBlkOpKind)
    {
        case GenTreeBlk::BlkOpKindCpObjUnroll:
            assert(!blkOp->gtBlkOpGcUnsafe);
            genCodeForCpObj(blkOp->AsBlk());
            break;

        case GenTreeBlk::BlkOpKindLoop:
            assert(!isCopyBlk);
            genCodeForInitBlkLoop(blkOp);
            break;

        case GenTreeBlk::BlkOpKindNativeOpcode:
            genConsumeOperands(blkOp);
            // Emit the size constant expected by the memory.copy and memory.fill opcodes
            GetEmitter()->emitIns_I(INS_i32_const, EA_4BYTE, blkOp->Size());
            GetEmitter()->emitIns_I(isCopyBlk ? INS_memory_copy : INS_memory_fill, EA_8BYTE, /* memory index */ 0);
            break;

        default:
            unreached();
    }

Review comment (suggestion):
```suggestion
```
All of these asserts are not very relevant with WASM's `EMIT_GC_INFO == false`.

Code after:
            break;
        case PackOperAndType(GT_GT, TYP_DOUBLE):
            ins = INS_f64_gt;
            break;
        default:
            unreached();
    }

    GetEmitter()->emitIns(ins);

    if (invertSense)
    {
        GetEmitter()->emitIns(INS_i32_eqz);
    }

    WasmProduceReg(treeNode);
}

//------------------------------------------------------------------------
// genCodeForStoreBlk: Produce code for a GT_STORE_BLK node.
//
// Arguments:
//    blkOp - the node
//
void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp)
{
    assert(blkOp->OperIs(GT_STORE_BLK));

    bool isCopyBlk = blkOp->OperIsCopyBlkOp();

    switch (blkOp->gtBlkOpKind)
    {
        case GenTreeBlk::BlkOpKindCpObjUnroll:
            genCodeForCpObj(blkOp->AsBlk());
            break;

        case GenTreeBlk::BlkOpKindLoop:
            assert(!isCopyBlk);
            genCodeForInitBlkLoop(blkOp);
            break;

        case GenTreeBlk::BlkOpKindNativeOpcode:
            genConsumeOperands(blkOp);
            // Emit the size constant expected by the memory.copy and memory.fill opcodes
            GetEmitter()->emitIns_I(INS_i32_const, EA_4BYTE, blkOp->Size());
            GetEmitter()->emitIns_I(isCopyBlk ? INS_memory_copy : INS_memory_fill, EA_8BYTE, LINEAR_MEMORY_INDEX);
            break;

        default:
            unreached();
    }
Repository: dotnet/runtime
File: src/coreclr/tools/Common/Compiler/ObjectWriter/WasmNative.cs (C#)

Code before:
            pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), Min);
            if (LimitType == WasmLimitType.HasMinAndMax)
            {
                pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), Max!.Value);
            }
            return pos;
        }

        public override int EncodeSize()
        {
            uint size = 1 + DwarfHelper.SizeOfULEB128(Min);
            if (LimitType == WasmLimitType.HasMinAndMax)
            {
                size += DwarfHelper.SizeOfULEB128(Max!.Value);
            }
            return (int)size;
        }
    }

    public class WasmImport : IWasmEncodable
    {
        public string Module;
        public string Name;
        public WasmExternalKind Kind;
        public int? Index;
        WasmImportType Import;

        public WasmImport(string module, string name, WasmExternalKind kind, WasmImportType import, int? index = null)
        {
            Module = module;
            Name = name;
            Kind = kind;
            Import = import;
            Index = index;
        }

        public int Encode(Span<byte> buffer) => Import.Encode(buffer);
        public int EncodeSize() => Import.EncodeSize();

#nullable disable
    }
}


Review comment (suggestion):
`WasmImport` exposes mutable public fields (`Module`, `Name`, `Kind`, `Index`). In this codebase, similar DTO-like types use properties instead (e.g. `CoffObjectWriter.CoffSectionHeader` in `CoffObjectWriter.cs:574+`, `MachObjectWriter.MachSegment64Header` in `MachObjectWriter.cs:918+`). Consider switching these to properties (ideally get-only / init-only) or making the type `internal` to avoid analyzer violations (e.g. CA1051) and accidental mutation.

Code after:
        public override int Encode(Span<byte> buffer)
        {
            int pos = 0;
            buffer[pos++] = (byte)_limitType;
            pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), _min);
            if (_limitType == WasmLimitType.HasMinAndMax)
            {
                pos += DwarfHelper.WriteULEB128(buffer.Slice(pos), _max!.Value);
            }
            return pos;
        }

        public override int EncodeSize()
        {
            uint size = 1 + DwarfHelper.SizeOfULEB128(_min);
            if (_limitType == WasmLimitType.HasMinAndMax)
            {
                size += DwarfHelper.SizeOfULEB128(_max!.Value);
            }
            return (int)size;
        }
    }

    public class WasmImport : IWasmEncodable
    {
        public readonly string Module;
        public readonly string Name;
        public  WasmExternalKind Kind => Import.Kind;
        public readonly int? Index;
        public readonly WasmImportType Import;

        public WasmImport(string module, string name, WasmImportType import, int? index = null)
        {
            Module = module;
            Name = name;
            Import = import;
            Index = index;
        }

        public int Encode(Span<byte> buffer) => Import.Encode(buffer);
        public int EncodeSize() => Import.EncodeSize();
    }
}
Repository: dotnet/runtime
File: src/coreclr/tools/Common/Compiler/ObjectWriter/WasmNative.cs (C#)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Linq;
using System.Collections.Generic;
using ILCompiler.ObjectWriter.WasmInstructions;

namespace ILCompiler.ObjectWriter
{
    public interface IWasmEncodable
    {
        int EncodeSize();
        int Encode(Span<byte> buffer);
    }

    public enum WasmSectionType
    {
        Custom = 0,
        Type = 1,
        Import = 2,
        Function = 3,
        Table = 4,
        Memory = 5,
        Global = 6,
        Export = 7,
        Start = 8,
        Element = 9,
        Code = 10,
        Data = 11,
        DataCount = 12,
        Tag = 13,

Review comment (suggestion):
This `using` appears to be unused in this file and will trigger CS8019 (treated as an error in this repo). Remove the unused `using ILCompiler.ObjectWriter.WasmInstructions;` directive.
```suggestion

```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Linq;
using System.Collections.Generic;

namespace ILCompiler.ObjectWriter
{
    public interface IWasmEncodable
    {
        int EncodeSize();
        int Encode(Span<byte> buffer);
    }

    public enum WasmSectionType
    {
        Custom = 0,
        Type = 1,
        Import = 2,
        Function = 3,
        Table = 4,
        Memory = 5,
        Global = 6,
        Export = 7,
        Start = 8,
        Element = 9,
        Code = 10,
        Data = 11,
        DataCount = 12,
        Tag = 13,
    }
Repository: dotnet/runtime
File: src/coreclr/tools/Common/Compiler/ObjectWriter/WasmNative.cs (C#)

Code before:
            }
            return (int)size;
        }
    }

    public class WasmImport : IWasmEncodable
    {
        public readonly string Module;
        public readonly string Name;
        public readonly WasmExternalKind Kind;
        public readonly int? Index;
        public readonly WasmImportType Import;

        public WasmImport(string module, string name, WasmExternalKind kind, WasmImportType import, int? index = null)
        {
            Module = module;
            Name = name;
            Kind = kind;
            Import = import;
            Index = index;
        }

        public int Encode(Span<byte> buffer) => Import.Encode(buffer);
        public int EncodeSize() => Import.EncodeSize();

#nullable disable
    }
}


Review comment (suggestion):
`#nullable disable` is placed inside `WasmImport` and (as written) disables nullability for the remainder of the file without an obvious reason. This can hide nullability warnings in future edits; consider removing it or scoping it more explicitly (e.g., disable/restore around the specific code that needs it).
```suggestion

```

Code after:
        {
            uint size = 1 + DwarfHelper.SizeOfULEB128(_min);
            if (_limitType == WasmLimitType.HasMinAndMax)
            {
                size += DwarfHelper.SizeOfULEB128(_max!.Value);
            }
            return (int)size;
        }
    }

    public class WasmImport : IWasmEncodable
    {
        public readonly string Module;
        public readonly string Name;
        public  WasmExternalKind Kind => Import.Kind;
        public readonly int? Index;
        public readonly WasmImportType Import;

        public WasmImport(string module, string name, WasmImportType import, int? index = null)
        {
            Module = module;
            Name = name;
            Import = import;
            Index = index;
        }

        public int Encode(Span<byte> buffer) => Import.Encode(buffer);
        public int EncodeSize() => Import.EncodeSize();
    }
}
Repository: dotnet/runtime
File: src/coreclr/vm/wasm/calldescrworkerwasm.cpp (C++)

Code before:
    // revisited to ensure correctness.

    MethodDesc* pMethod = PortableEntryPoint::GetMethodDesc(pCallDescrData->pTarget);
    InterpByteCodeStart* targetIp = pMethod->GetInterpreterCode();
    if (targetIp == NULL)
    {
        GCX_PREEMP();
        (void)pMethod->DoPrestub(NULL /* MethodTable */, CallerGCMode::Coop);
        targetIp = pMethod->GetInterpreterCode();
    }

    size_t argsSize = pCallDescrData->nArgsSize;
    void* retBuff;
    int8_t* args;
    if (pCallDescrData->hasRetBuff)
    {
        argsSize -= INTERP_STACK_SLOT_SIZE;
        if (pCallDescrData->hasThis)
        {
            retBuff = *SPECIAL_ARG_ADDR(1);
            *SPECIAL_ARG_ADDR(1) = *SPECIAL_ARG_ADDR(0);
        } else {
            retBuff = *SPECIAL_ARG_ADDR(0);
        }
        args = (int8_t*)SPECIAL_ARG_ADDR(1);
    } else {
        args = (int8_t*)pCallDescrData->pSrc;
        retBuff = pCallDescrData->returnValue;
    }

    ExecuteInterpretedMethodWithArgs((TADDR)targetIp, args, argsSize, retBuff, (PCODE)&CallDescrWorkerInternal);
}


Review comment (suggestion):
```suggestion
        }
        else
        {
            retBuff = *SPECIAL_ARG_ADDR(0);
        }
        args = (int8_t*)SPECIAL_ARG_ADDR(1);
    }
    else
    {
```

Code after:
    // revisited to ensure correctness.

    MethodDesc* pMethod = PortableEntryPoint::GetMethodDesc(pCallDescrData->pTarget);
    InterpByteCodeStart* targetIp = pMethod->GetInterpreterCode();
    if (targetIp == NULL)
    {
        GCX_PREEMP();
        (void)pMethod->DoPrestub(NULL /* MethodTable */, CallerGCMode::Coop);
        targetIp = pMethod->GetInterpreterCode();
    }

    size_t argsSize = pCallDescrData->nArgsSize;
    void* retBuff;
    int8_t* args;
    if (pCallDescrData->hasRetBuff)
    {
        argsSize -= INTERP_STACK_SLOT_SIZE;
        if (pCallDescrData->hasThis)
        {
            retBuff = *SPECIAL_ARG_ADDR(1);
            *SPECIAL_ARG_ADDR(1) = *SPECIAL_ARG_ADDR(0);
        }
        else
        {
            retBuff = *SPECIAL_ARG_ADDR(0);
        }
        args = (int8_t*)SPECIAL_ARG_ADDR(1);
    }
    else
    {
        args = (int8_t*)pCallDescrData->pSrc;
        retBuff = pCallDescrData->returnValue;
    }

    ExecuteInterpretedMethodWithArgs((TADDR)targetIp, args, argsSize, retBuff, (PCODE)&CallDescrWorkerInternal);
}
Repository: dotnet/runtime
File: src/coreclr/vm/jithelpers.cpp (C++)

Code before:
        refHandle.Set(pMT->GetGCThreadStaticsBasePointer());
    }
    else
    {
        refHandle.Set(pMT->GetNonGCThreadStaticsBasePointer());
    }

    END_QCALL;
}

// *** This helper corresponds CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_NOCTOR_OPTIMIZED2.
HCIMPL1(void*, JIT_GetNonGCThreadStaticBaseOptimized2, UINT32 staticBlockIndex)
{
    FCALL_CONTRACT;

    return ((BYTE*)&t_ThreadStatics) + staticBlockIndex;
}
HCIMPLEND

// *** This helper corresponds CORINFO_HELP_READYTORUN_THREADLOCALBASE.
// Returns the address of the thread local data structure (t_ThreadStatics)
HCIMPL0(void*, JIT_GetThreadLocalBase)
{
    FCALL_CONTRACT;

    return (void*)&t_ThreadStatics;
}
HCIMPLEND

#include <optdefault.h>

//========================================================================
//
//      CASTING HELPERS
//
//========================================================================

static BOOL ObjIsInstanceOfCore(Object *pObject, TypeHandle toTypeHnd, BOOL throwCastException)
{
    CONTRACTL {
        THROWS;
        GC_TRIGGERS;
        MODE_COOPERATIVE;
        PRECONDITION(CheckPointer(pObject));
    } CONTRACTL_END;

    BOOL fCast = FALSE;
    MethodTable* pMT = pObject->GetMethodTable();

    OBJECTREF obj = ObjectToOBJECTREF(pObject);
    GCPROTECT_BEGIN(obj);

Review comment (suggestion):
This should return the address of the pThread field inside of the t_ThreadStatics variable.

Code after:
        refHandle.Set(pMT->GetGCThreadStaticsBasePointer());
    }
    else
    {
        refHandle.Set(pMT->GetNonGCThreadStaticsBasePointer());
    }

    END_QCALL;
}

// *** This helper corresponds CORINFO_HELP_GETSHARED_NONGCTHREADSTATIC_BASE_NOCTOR_OPTIMIZED2.
HCIMPL1(void*, JIT_GetNonGCThreadStaticBaseOptimized2, UINT32 staticBlockIndex)
{
    FCALL_CONTRACT;

    return ((BYTE*)&t_ThreadStatics) + staticBlockIndex;
}
HCIMPLEND

// *** This helper corresponds CORINFO_HELP_GETDIRECTONTHREADLOCALDATA_NONGCTHREADSTATIC_BASE.
// Returns the address of the pThread field within thread-local storage, which serves as the storage
// location for the DirectOnThreadLocalData.pNativeThread thread static field. Adjust the value by
// OFFSETOF__CORINFO_Array__data, since all thread static bases are returned as relative to the start of an
// array object.
HCIMPL0(void*, JIT_GetDirectOnThreadLocalDataNonGCThreadStaticBase)
{
    FCALL_CONTRACT;

    return (void*)((uint8_t*)&(((ThreadLocalData*)&t_ThreadStatics)->pThread) - OFFSETOF__CORINFO_Array__data);
}
HCIMPLEND

#include <optdefault.h>

//========================================================================
//
//      CASTING HELPERS
//
//========================================================================

static BOOL ObjIsInstanceOfCore(Object *pObject, TypeHandle toTypeHnd, BOOL throwCastException)
{
    CONTRACTL {
        THROWS;
        GC_TRIGGERS;
        MODE_COOPERATIVE;
        PRECONDITION(CheckPointer(pObject));
    } CONTRACTL_END;

    BOOL fCast = FALSE;
    MethodTable* pMT = pObject->GetMethodTable();
Repository: dotnet/runtime
File: src/coreclr/vm/syncclean.cpp (C++)

Code before:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.


#include "common.h"

#include "syncclean.hpp"
#include "virtualcallstub.h"
#include "threadsuspend.h"

#ifdef FEATURE_INTERPRETER
void InterpDispatchCache_ReclaimAll();
#endif

VolatilePtr<Bucket> SyncClean::m_HashMap = NULL;
VolatilePtr<EEHashEntry*> SyncClean::m_EEHashTable;

void SyncClean::Terminate()
{
    CONTRACTL {
        NOTHROW;
        GC_NOTRIGGER;
    } CONTRACTL_END;

    CleanUp();
}

void SyncClean::AddHashMap (Bucket *bucket)
{
    WRAPPER_NO_CONTRACT;

    if (!g_fEEStarted) {
        delete [] bucket;
        return;
    }

    _ASSERTE (GetThreadNULLOk() == NULL || GetThread()->PreemptiveGCDisabled());

Review comment (performance):
The forward declaration for InterpDispatchCache_ReclaimAll could be moved to a header file (e.g., interpexec.h) to make it more maintainable and discoverable. This would avoid duplication if other parts of the codebase need to reference this function in the future, and follows the general pattern of declaring public functions in headers rather than using forward declarations at call sites.
```suggestion
#include "interpexec.h"
```

Code after:
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.


#include "common.h"

#include "syncclean.hpp"
#include "virtualcallstub.h"
#include "threadsuspend.h"

#ifdef FEATURE_INTERPRETER
#include "interpexec.h"
#endif

VolatilePtr<Bucket> SyncClean::m_HashMap = NULL;
VolatilePtr<EEHashEntry*> SyncClean::m_EEHashTable;

void SyncClean::Terminate()
{
    CONTRACTL {
        NOTHROW;
        GC_NOTRIGGER;
    } CONTRACTL_END;

    CleanUp();
}

void SyncClean::AddHashMap (Bucket *bucket)
{
    WRAPPER_NO_CONTRACT;

    if (!g_fEEStarted) {
        delete [] bucket;
        return;
    }

    _ASSERTE (GetThreadNULLOk() == NULL || GetThread()->PreemptiveGCDisabled());