Conversation
nihalpasham
left a comment
There was a problem hiding this comment.
The reported integer-redux/f32 failure is real, but splitting by lines leaves the same root cause in place. LLVM accepts two declarations on one line; an integer redux declaration followed by an f32 shuffle declaration still incorrectly selects ReduxF32. An unrelated .f32 SSA name can trigger it too.
Please match the actual intrinsic callee or PTX opcode/modifiers, with negative tests for unrelated names and same-line declarations. The submitted regression passes, but the independent counterexample also passes LLVM verification and still fails feature detection.
This partially addresses #1303; it does not close it yet.
6d99bc2 to
5ad1fed
Compare
|
You were right, and the line split was worse than not fixing it. Force-pushed a different change: the granularity approach is gone, and the predicate now matches the instruction. Both of your counterexamples, plus the one they led me toSame six inputs through the real
Five of six wrong today, and the last two are the ones I did not expect: the intrinsic spelling of a float reduction contains no So a module reducing floats through that path is detected only when an unrelated Stated as latent, not live: I have not produced a mis-targeted build from it. What it does nowTwo matchers, one per spelling that reaches the IR:
#1303 stays open as you said — the other conjunctive predicates still ask their question over module-scope text, and I still have no reproducer for any of them, so I have not touched them. |
|
Retracting half of my last comment. I said the intrinsic spelling being invisible to this predicate was "a false negative in the direction that matters", and promised to settle on hardware whether it was live. It is not, and the claim was wrong. I built Its IR is what I expected — eight float callees, no type suffix, and So the predicate really is blind to that spelling. What I missed is that it is not the only thing selecting the target: What stands, unchanged:
Nothing in the diff changes; only the claim does. |
5ad1fed to
b244421
Compare
nihalpasham
left a comment
There was a problem hiding this comment.
The narrower redux change fixes the earlier same-line and .f32 SSA cases, and it now recognizes unquoted float intrinsics.
There are still two lexical gaps at b244421d:
- An ordinary data string, comment or metadata value containing
@llvm.nvvm.redux.sync.fminnow triggers ReduxF32 and selects sm_100a. The@prefix does not make the text a callee. This false positive is new. - In external LLVM without generated catalog requirements, a real call to
@"llvm.nvvm.redux.sync.fmin", including its escaped-identifier spelling, misses ReduxF32 and selects sm_80.
The catalog requirement protects the typed Rust intrinsic path, as you clarified. LLVM 23 verifies these external-LLVM probes, and the public detector/target selector reproduce both results despite all 1,260 ordinary tests passing. Please distinguish actual decoded intrinsic symbols and inline-PTX instructions from comments and data, with regressions for these cases.
This is progress on #1303, but it still needs changes before landing.
b244421 to
135327f
Compare
|
Both real, and the first one I introduced. Fixed by giving the predicate a lexical pass over the IR so it is asked about code rather than about text. The same nine inputs, previous revision against this one
Seven of nine. Your two are in there; the two PTX rows are the same point generalized — I took "distinguish instructions from comments and data" to cover the inline-PTX half as well, since a What it does
Blanking rather than deleting keeps newlines and token boundaries where they were, so nothing can be made to match across a hole. LLVM writes a literal quote inside a string as The normalization is used by this predicate only. The other conjunctive predicates in that file still ask their question over raw module text, I still have no reproducer for any of them, and #1303 stays open for that. TestsThe nine rows above, as two tests —
|
nihalpasham
left a comment
There was a problem hiding this comment.
The module-wide false positive is fixed, but the new normalization still loses lexical context. At 135327f4, four LLVM-verified inputs are misclassified:
| Input | Expected | Selected |
|---|---|---|
asm sideeffect unwind "redux.sync.min.f32 ..." |
float redux | sm_80 |
Inline PTX with \72edux.sync.min.f32 (\72 is r) |
float redux | sm_80 |
// redux.sync.min.f32 ... inside an asm template |
no redux | sm_100a |
Ordinary symbol @"unused @llvm.nvvm.redux.sync.fmin" |
no redux | sm_100a |
The unwind and ordinary-symbol cases are regressions from the original predicate. The other two show why preserving the raw template is insufficient.
Please retain token identity instead of flattening decoded symbols and asm into one search string. Decode LLVM template escapes, recognize the complete asm modifier sequence, and inspect PTX instructions after excluding PTX comments. Add these controls beside the existing tests.
The unmodified codegen suite passes serially; the independent controls fail. One earlier parallel run hit an unrelated fake-opt executable-busy error. This needs changes before #1304 or its dependent #1305 can land. #1303 still covers the broader detector work.
135327f to
279ffe0
Compare
|
All four reproduce exactly as you described on Your four, plus the neighbours they impliedSame nine inputs,
The first five fail on What it does nowThe IR is lexed once — comments dropped, quoted text consumed whole before anything inside it is read — and the predicate asks two questions of the tokens:
Decode-then-strip is what the escaped-newline row checks: the raw template is one line, so stripping Every control is valid IR — and two of mine were notI checked all 23 controls across the three tests with Mutation-checked, which caught a hollow controlSeven mutations, each asserted to have applied: dropping
One scope note: a bare word that is itself the instruction also counts, because an existing upstream test feeds a raw PTX fragment ( |
nihalpasham
left a comment
There was a problem hiding this comment.
The four earlier cases now pass at 279ffe0f. Keeping LLVM token identity fixes those boundaries. Two issues remain in the PTX template scan:
| Valid PTX | Expected | Selected |
|---|---|---|
redux.sync.min .f32 $0, $1, $2; |
float redux | sm_80 |
redux.sync.min/* qualifier */.f32 $0, $1, $2; |
float redux | sm_80 |
redux .sync .min .f32 $0, $1, $2; |
float redux | sm_80 |
.file 1 "name redux.sync.min.f32 label.cu" |
no redux | sm_100a |
These are complete LLVM modules, not malformed snippets. All four pass llvm-as, llc and CUDA 13.4 ptxas at sm_100a. The quoted-file control also assembles at sm_80; the spaced reduction fails there with Instruction 'redux.f32' not supported on .target 'sm_80'. The actual crate reproduces all four wrong selections.
Please recognize the PTX opcode and modifiers across whitespace/comments, and keep quoted directive data out of instruction detection. Add these controls to the tests. Keep the helper comments focused on their current contract.
The unmodified codegen suites pass (1,263 tests/doctests), as do strict Clippy and formatting. The independent PTX controls fail. #1304 and its dependent #1305 still need changes; the original four LLVM cases are resolved.
…nearby text `contains_redux_f32_features` asked whether `redux.sync` and `.f32` both appeared in one `;`-delimited piece of the LLVM IR. `;` starts a comment in IR, so that piece is usually the whole module: a module reducing integers with any unrelated `.f32` was pushed from sm_80 to sm_100a. The integer `warp_reduce` fast path in NVlabs#811 is such a module. The predicate now decides on tokens. The IR is lexed with quoted text consumed whole and comments dropped, and two things count: * a global symbol, quoting decoded, named `llvm.nvvm.redux.sync.` followed by a float operation -- the ones beginning with `f`, since the float callees carry no type suffix; * a `redux` opcode whose modifiers, in an inline-asm template, start with `.sync` and include `.f32`. The template is the string after `LLParser`'s `asm [sideeffect] [alignstack] [inteldialect] [unwind]`, decoded as `llvm::UnEscapeLexed` does, then lexed as PTX: whitespace and `//` and `/* */` comments separate tokens without ending a modifier run, a number consumes its whole run, and a string literal -- in which a backslash is literal -- is skipped as data. So text that only mentions the instruction or the callee -- in a comment, a data or metadata string, a `.file` path, or another symbol's quoted name -- no longer counts, and the quoted, escaped, `unwind`, spaced and comment-separated spellings of a real one do. Thirty-one controls across four tests, each a line from a module `llvm-as` from the pinned toolchain accepts. All but one also lower through `llc`; the exception is a global whose quoted name contains a space and `@`, which is valid IR that NVPTX cannot emit, and which tests the lexer rather than lowering. The PTX controls are also checked with ptxas, where the float cases assemble at sm_100a and are refused at sm_80 as `redux.f32`. Twelve mutations of the lexers each fail a test. The float intrinsic spelling was never matched by the old predicate, but that is not a live mis-target: those are catalog intrinsics, and `select_target_with_generated` takes sm_100a from their recorded requirements regardless -- `redux_f32` selects sm_100a with nothing pinned under either version. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
279ffe0 to
6b4ac7d
Compare
|
All four reproduce on What changedTemplates are now lexed as PTX rather than split on whitespace. Whitespace and Two lexer rules came from checking ptxas rather than from the PTX spec, and both were bugs in my first pass:
Controls,
|
| input | want | 279ffe0f |
now |
|---|---|---|---|
redux.sync.min .f32 |
ReduxF32 | none | ReduxF32 |
redux.sync.min/* qualifier */.f32 |
ReduxF32 | none | ReduxF32 |
redux .sync .min .f32 |
ReduxF32 | none | ReduxF32 |
.file 1 "name redux.sync.min.f32 label.cu" |
none | ReduxF32 | none |
redux.sync.min\n.f32 |
ReduxF32 | none | ReduxF32 |
.pragma "nounroll\"; then a real float redux |
ReduxF32 | ReduxF32 | ReduxF32 |
redux .sync .min .u32 (spaced integer) |
none | none | none |
{ .reg .f32 t; redux.sync.min.u32 …; mov.f32 t, …; } |
none | none | none |
The last three pass under both. They pin rules the new lexer has to keep: strings close at the next quote, spacing does not make an integer reduction float, and .f32 counts only as a modifier of the redux opcode, not anywhere in the template. That last one was missing until a mutation replacing the modifier check with ".f32 anywhere" survived; it is caught now.
Twelve mutations across the IR and PTX lexers, each asserted to have applied, each failing a test.
One claim I corrected before posting
I had written that every control lowers through llc. Checking all 31 (not just this round's): 30 do. The exception is your earlier @"unused @llvm.nvvm.redux.sync.fmin" global, which is valid IR that NVPTX refuses to emit ("Symbol name with unsupported characters"). It still does its job — it tests that a quoted name stays one token — and the commit message now says so rather than overclaiming.
Comments are trimmed to the current contract. cargo test -p cuda-oxide-codegen 1264 pass across all suites (your 1263 plus the new test); clippy -D warnings, fmt and all 17 guards clean. #1305 is rebased onto this.
Fixes #1303 for the
redux.synccase it was filed for.contains_redux_f32_featuresasked whetherredux.syncand.f32appeared in one;-delimited piece of the LLVM IR.;starts a comment in IR, so that piece is usually the whole module, and a module reducing integers with any unrelated.f32was pushed from sm_80 to sm_100a. The integerwarp_reducefast path in #811 is such a module.How
The predicate decides on tokens.
llvm.nvvm.redux.sync.followed by a float operation — thef-prefixed ones, since the float callees carry no type suffix.LLParser'sasm [sideeffect] [alignstack] [inteldialect] [unwind], decoded asllvm::UnEscapeLexeddoes, then lexed as PTX. Areduxopcode counts when its modifier run — the.nametokens after it — starts with.syncand includes.f32. Whitespace and comments do not end the run; string literals are skipped as data, with a backslash literal inside them; a number consumes its whole run.Text that only mentions the instruction or callee — a comment, a data or metadata string, a
.filepath, another symbol's quoted name — no longer counts. The quoted, escaped,unwind, spaced and comment-separated spellings of a real one do.Tests
31 controls in four tests, each a line from a module
llvm-asfrom the pinned toolchain accepts; 30 also lower throughllc(the exception is a global whose quoted name contains a space and@, which NVPTX cannot emit). The PTX controls are checked with ptxas: the float cases assemble at sm_100a and are refused at sm_80 asredux.f32. Twelve mutations of the lexers each fail a test.cargo test -p cuda-oxide-codegen1264 pass with the existing detection tests untouched; clippy-D warnings, fmt and all 17scripts/check-*.shclean.The float intrinsic spelling was never matched by the old predicate, but that is not a live mis-target: those are catalog intrinsics, so
select_target_with_generatedtakes sm_100a from their recorded requirements regardless.#1303 stays open for the other conjunctive predicates, which still read raw module text.