dotnet / dotnet/runtime

JIT: SupportsSettingZeroFlag xarch path should query instruction tables instead of hardcoding op lists

Open
#129,346 2 comments 1 reaction 1 assignee Claimed by @AndyAyersMS View on GitHub
area-CodeGen-coreclr
Dominant language
C#
Stars
18.3k
Forks
5.6k
PR merge metrics
PR metrics pending

Description

## Background

In #129288 (fixed by the minimal removal of `GT_ROL`/`GT_ROR` from the
shift group), @tannergooding [pointed out](https://github.com/dotnet/runtime/issues/129288#issuecomment-...)
that hardcoded op→flag lists in `GenTree::SupportsSettingZeroFlag()`
duplicate information that's already accurately tracked in the
xarch instruction tables, and risk drifting from reality:

> We notably have this info in the instruction tables. For example, see
> [`instrsxarch.h#L1258-L1260`](https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/instrsxarch.h#L1258-L1260),
> where `ror` is tracked as `Undefined_OF | Writes_CF` which means
> other flags (like `ZF`) are left "as is".
>
> It might be goodness to have most of the trivial arithmetic ops like
> this rather query the instruction codegen would emit and lookup the
> actual flags, rather than hardcode them all. Particularly because
> that may get out of sync, be missed when we fixup the general
> tables, or in the future if/when we change the emit strategy for a
> given node kind.
>
> Codegen itself typically has such a mapping of `Oper -> ins` already
> and while there is some nuance, the places we typically care about
> such queries (lowering) should know statically what codegen will
> emit (as this is required for containment and other LSRA required
> flags to be correct)

This issue tracks doing that refactor for the xarch path of
`SupportsSettingZeroFlag()`.

## Current state (after #129288 fix)

```cpp
#if defined(TARGET_XARCH)
if (OperIs(GT_LSH, GT_RSH, GT_RSZ))
{
// Shift instructions do not update the flags in case of count being zero.
return gtGetOp2()->IsNeverZero();
}
// Note: GT_ROL / GT_ROR are intentionally NOT in this list. On xarch,
// ROL/ROR/RCL/RCR only update CF (and OF in the 1-bit form); SF/ZF/AF/PF
// are not affected for any count.

if (OperIs(GT_AND, GT_OR, GT_XOR, GT_ADD, GT_SUB, GT_NEG))
{
return true;
}

#ifdef FEATURE_HW_INTRINSICS
if (OperIs(GT_HWINTRINSIC) && emitter::DoesWriteZeroFlag(HWIntrinsicInfo::lookupIns(AsHWIntrinsic(), nullptr)))
{
return true;
}
#endif
#endif
```

The hardcoded scalar list `(GT_AND, GT_OR, GT_XOR, GT_ADD, GT_SUB,
GT_NEG)` and the separate shift list still mirror data that already
exists in `instrsxarch.h`. The `GT_HWINTRINSIC` branch already shows
the cleaner pattern.

## Building blocks already in tree

- `instrsxarch.h` carries accurate per-instruction `insFlags`
(`Writes_ZF`, `Undefined_OF`, `Resets_OF`, etc.).
- `emitter::DoesWriteZeroFlag(instruction ins)` in
[`emitxarch.cpp`](https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/emitxarch.cpp#L735)
already queries it.
- `CodeGen::genGetInsForOper(genTreeOps oper, var_types type)` in
[`codegenxarch.cpp`](https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/codegenxarch.cpp#L4468)
already maps `GT_ADD, GT_AND, GT_LSH, GT_MUL, GT_NEG, GT_NOT, GT_OR,
GT_ROL, GT_ROR, GT_RSH, GT_RSZ, GT_SUB, GT_XOR, ...` to the right
`instruction`. It uses no instance state, so making it static is
straightforward.

## Proposed shape

```cpp
#if defined(TARGET_XARCH)
// For arith opers whose lowering emits a single primary instruction
// we can name statically, defer to the instruction tables.
if (OperIs(GT_ADD, GT_AND, GT_OR, GT_XOR, GT_SUB, GT_NEG,
GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR))
{
var_types type = TypeGet();
if (!varTypeIsFloating(type) && !varTypeIsSIMD(type))
{
instruction ins = CodeGen::GetIns_FromOper(OperGet(), type);
if (ins != INS_invalid && !emitter::DoesWriteZeroFlag(ins))
{
return false;
}
// Shifts only write flags when count != 0 (Intel SDM).
// The flag tables can't express the count-dependent
// condition; encode it here.
if (OperIs(GT_LSH, GT_RSH, GT_RSZ))
{
return gtGetOp2()->IsNeverZero();
}
return true;
}
}
#ifdef FEATURE_HW_INTRINSICS
if (OperIs(GT_HWINTRINSIC) && emitter::DoesWriteZeroFlag(HWIntrinsicInfo::lookupIns(AsHWIntrinsic(), nullptr)))
{
return true;
}
#endif
#endif
```

Two variants for getting the `Oper -> ins` mapping callable from
`gentree.cpp`:

- **2a**: Refactor `genGetInsForOper` into a static helper
(`CodeGen::GetIns_FromOper`) since it uses no instance state.
- **2b**: Add a small local `GetXArchArithIns(genTreeOps, var_types)`
in `instr.cpp` covering just the arith opers we care about,
returning `INS_invalid` otherwise. Avoids the codegen.h API churn at
the cost of a small duplicated table.

## Note on shift semantics

The instruction tables don't model the Intel SDM "if count is 0 the
flags are unchanged" rule for `shl`/`shr`/`sar` — they show
`Writes_ZF` unconditionally. The existing
`gtGetOp2()->IsNeverZero()` guard must be preserved for the shift
case; the table query gates on `DoesWriteZeroFlag(ins)` being true,
but the count-dependent special case still requires the `IsNeverZero`
check. This is the only non-trivial nuance in the proposed refactor.

## Outcome

- Removes the hardcoded `GT_AND/GT_OR/GT_XOR/GT_ADD/GT_SUB/GT_NEG`
scalar list (and the shift list) in favor of a single
table-driven check.
- Catches future drift automatically when the instruction tables
change.
- Aligns the scalar path with the already-existing `GT_HWINTRINSIC`
pattern in the same function.

cc @tannergooding @jakobbotsch @EgorBo (last touched
`SupportsSettingZeroFlag` in #117960)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.