llvm / llvm/llvm-project

[AMDGPU] Making implicit narrowing conversions explicit — tracking issue

Open
#215,213 1 comment 0 reactions 0 assignees View on GitHub
backend:AMDGPU metaissue
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

This issue describes a set of small NFC patches that fix existing implicit integer narrowing conversions in the AMDGPU backend. The fixes change types or add explicit static casts. Each patch covers a few related files and stands on its own to make review easier.

This issue exists to clarify the limited scope of the changes, facilitate discussions, and track progress of the patches.

This effort has not uncovered any bugs caused by implicit narrowing conversions. Every site changed is safe today; the value is that the next change that incorrectly narrows a type will be easier to spot.

### Scope

AMDGPU backend of LLVM can be built using MSVC as part of a graphics compilation pipeline.
This set of patches focuses on the following MSVC warnings:
- C4244 "conversion from 'T1' to 'T2', possible loss of data"
- C4267 "conversion from 'size_t' to 'T'"

These MSVC warnings are disabled in LLVM builds by default. Enabling these warnings is required in some situations. The proposed patches fix the code to clear these warnings in AMDGPU specific files and a small number of lines in generic LLVM code used by the AMDGPU backend. Clearing the bulk of the warnings will make it easier to carry out regular security compliance checks.

**This is not a proposal to enable the warnings by default.** The PRs do not remove `-wd4244`/`-wd4267` from `HandleLLVMOptions.cmake` or change any defaults. The LLVM community is not asked to proactively keep the code clean of these warnings going forward. New instances will show up with future patches and the community is not expected to spend any effort on them.

A separate discussion would be needed in the future if we consider enabling these warnings by default. While this set of PRs is a step in that direction, it does not attempt to address the gaps raised by previous discussions about enabling MSVC warnings by default: https://discourse.llvm.org/t/msvc-suppressed-warnings-should-they-be-actually-suppressed/76939/2

**Other compilers** The Clang counterparts are `-Wimplicit-int-conversion` (closest to C4244) and `-Wshorten-64-to-32` (closest to C4267). These Clang warnings are disabled by default, except in `libc`. Similar number of warnings was produced by `VS 2022` with `/w14244 /w14267` and `Clang 22.1.8` with `-Wimplicit-int-conversion -Wno-sign-conversion -Wshorten-64-to-32` enabled. The proposed patches that clear MSVC warnings in AMDGPU files, also clear almost all Clang warnings in these files.

A range of bug-prone narrowing conversions can also be reported by `clang-tidy` (see https://clang.llvm.org/extra/clang-tidy/checks/bugprone/narrowing-conversions.html). GCC's closest option is `-Wconversion`.

**Other MSVC warnings** A separate set of patches similarly clears MSVC warnings C4703/C4319 (potentially uninitialized local pointers) in AMDGPU specific files (https://github.com/llvm/llvm-project/pull/208469) and generic LLVM code https://github.com/llvm/llvm-project/pull/208565, where reviewers requested an RFC to discuss the desirability of the changes.

For narrowing conversions, the code clarity seems an easy win and the limited scope of the changes does not seem to justify an RFC process, but I'll be happy to make one, if reviewers prefer.

### The patches

The following tables show the number of warnings fixed by each patch.

They count unique source locations based on `file(line)` in warnings reported during the build. Grepping the raw build log gives many more, because a warning in a header is re-emitted by every translation unit that includes it.

**TableGen** Small changes in code that generates .inc files for all targets.

| PR | description | C4244 | C4267 |
|---|---|---:|---:|
| #215210 | `Combine.td`: two GICombineRule match bodies | 2 | 2 |
| #215211 | `DecoderEmitter.cpp`: ULEB128 locals in emitted `decodeInstruction` | 8 | 0 |
| #215212 | Three other TableGen emitters: `size_t` in emitted matcher tables | 6+4 | 4+5 |
| | **total** | **16+4** | **6+5** |

Where a cell has two terms, they are the number of warnings in the AMDGPU tables + the number in the X86 and R600 tables.

**AMDGPU backend**

| PR | description | C4244 | C4267 |
|---|---|---:|---:|
| #215190 | disassembler decoder callback signatures | 5899 | 0 |
| #215191 | `.td` predicate and XForm bodies | 16 | 0 |
| #215192 | `AMDGPUBaseInfo` | 19 | 0 |
| #215193 | disassembler | 25 | 2 |
| #215194 | subtarget and analysis passes | 17 | 3 |
| #215195 | asm printing and MIR formatting | 39 | 1 |
| #215196 | libcalls, TTI and InstCombine | 35 | 3 |
| #215197 | hazard and waitcnt passes | 51 | 9 |
| #215198 | MC layer | 62 | 3 |
| #215199 | GlobalISel support code | 51 | 5 |
| #215200 | schedulers | 17 | 54 |
| #215201 | instruction selector | 64 | 3 |
| #215202 | assembly parser | 68 | 19 |
| #215203 | frame and register handling | 85 | 7 |
| #215204 | SelectionDAG lowering | 89 | 2 |
| #215205 | register bank info | 131 | 6 |
| #215206 | legalizer | 89 | 7 |
| #215207 | IR-level passes | 105 | 30 |
| #215208 | `SIInstrInfo` and peepholes | 100 | 14 |
| #215209 | `SIISelLowering` | 191 | 13 |
| | **total** | **7153** | **181** |

The first two rows are warnings in generated `.inc` files rather than in the hand-written sources; the fix is to the callback signatures and to the `.td` bodies that TableGen copies into the tables.

### Overall stats

No warnings in AMDGPU specific files remain, no new warnings in generic files, `check-llvm` passes with no unexpected failures.

pathC4244C4267
basetipfixedbasetipfixed

llvm/lib/Target/AMDGPU/ (excluding R600)12380100%1810100%
build/lib/Target/AMDGPU/*.inc (generated)59310100%60100%
AMDGPU subtotal - the scope of this series71690100%1870100%
build/lib/Target/X86/*.inc (generated)363211%30100%
R600 - llvm/lib/Target/AMDGPU/R600* and its tables (out of scope)51510%22209%
llvm/lib/Target/X86/111311130%2252250%
llvm/lib/ (all other subdirectories)506750670%283128310%
llvm/include/4544540%4014010%
llvm/tools/3333330%3683680%
llvm/utils/2422420%3253250%
MSVC STL / Windows SDK headers27270%10100%
other110%00n/a
whole build14493732049%437241804%

Base revision: `9a19c7750f949c979372ba1d2eec8a7a40061aef`
Compiler version: `MSVC 19.44.35228.0, toolset 14.44.35207, VS 2022 BuildTools`

Measured on a clean full build + check-llvm configured with
```
-DLLVM_COMPILE_FLAGS="/w14244;/w14267"
-DCMAKE_BUILD_TYPE=Release
-DLLVM_ENABLE_ASSERTIONS=ON
-DCMAKE_DISABLE_PRECOMPILE_HEADERS=ON
-DLLVM_TARGETS_TO_BUILD="X86;AMDGPU"
```

Everything still outstanding is generic LLVM code which is out of scope for this issue (`DAGCombiner.cpp`, `SelectionDAG.cpp`, `TargetLowering.cpp`, `MetadataLoader.cpp` and similar).

The stats and the fixes were generated with assistance from Claude, and cross-checked manually.

Contributor guide

Open the contributing guide

Research direction

Start with the listed AMDGPU patch series and the files under llvm/lib/Target/AMDGPU, then reproduce the configured MSVC warning build using the provided CMake flags. Work is divided across the referenced PRs by backend subsystem. Done means no C4244 or C4267 warnings remain in AMDGPU-specific files and check-llvm passes without unexpected failures.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp
Domain
compilers
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.