Reduce from an identity held in a register

[vswagath1989]

An f32 reduction could not be compiled. Three changes, each standing on its own.

1. An identity held in a register

A reduction's accumulator was filled with its identity in place. Where a device
turns such a fill into an instruction's immediate, the width of that field bounds
what the identity may be. An f16 negative infinity fits; an f32 one does not.

Giving a register its value up front has no such bound, and reaching it needs the
write to be invariant and clear of the loop. Neither held for the accumulator: the
reduction writes it as well as the fill, and the combiner reads a subview of it, so
nothing that hoists invariant writes would touch it.

So the identity goes to a buffer of its own, written once and never read back, and
the accumulator is copied from it:

  %identity = memref.alloc() : memref<1x32xf32, "REG">
  linalg.fill ins(%cst) outs(%identity)
  memref.copy %identity, %accumulator

Hoisting after the pass lifts that fill clear of the loop, which is what lets the
value be given to the register rather than carried in an instruction. The
accumulator is then copied from it once per reduction, which is what the identity
is for -- a register given its value up front is set once, and a reduction needs to
start from it again for every statistic.

2. A copy whose source is a buffer

memref.copy was lowered by storing its source straight into the destination.
That is right for a fifo read, which is already a value, and wrong for a buffer:

error: 'agen.vector_store' op operand #0 must be vector of any type values or
       Dataflow custom vector type, but got 'memref<1x32xf32>'

A buffer source is loaded first now. The pass above is the first thing to emit that
form.

3. A pattern that takes its precision from the IR

A template written for either width needs the caller to say which one, and the
callers that say it hardcode a precision per pattern -- so a pattern covering both
widths had to be written twice.

ktdf.with_precision(op, params) reads the element type the op accumulates in and
adds the precision to the pattern's own parameter dictionary, leaving the rest of
that dictionary alone. It fails for a type no template names, so a compute is left
as it is rather than lowered at the wrong width.

4. arith.minnumf had no lowering

The frontend legality check accepts minnumf alongside minimumf, maxnumf and
maximumf. Only minnumf had no lowering case, so a body using it passed the check
and died further down with nothing to go on:

error: unsupported operation type in linalg.generic body

It lowers to a plain min. There is no absolute-min operator, so unlike maxnumf
it has no second shape to select for an abs-of-both pattern.

5. A compare and a selection had no lowering

arith.cmpf and arith.select were in neither the allow-list nor the lowering, so a
comparison could not be expressed at all. Both dialect ops they map to already
existed, and so did the lowerings below them.

The compare's result carries the operands' type rather than a boolean -- that is what
a selection takes as its condition, and the i1 form of the op is its separate mask
operand. Only the ordered predicates map; an unordered one asks about NaN, which the
compare does not answer.

What this reaches depends on the shape:

  %c = arith.cmpf olt, %a, %b : f16
  %r = arith.select %c, %a, %b : f16      // fuses into one min, works end to end

  %c = arith.cmpf oge, %a, %b : f16
  %r = arith.select %c, %one, %zero : f16 // expressible, not yet compilable

A select whose two sides are the values compared fuses with the compare into a single
min or max further down. A select over anything else leaves the compare with no reader
there, so it stops with Dangling non-compute op has no use. Closing that is a change
below this repo, not in it.

Where things are

file	what
lib/Dialect/KTDF/Transforms/MapReductionPartials.cpp	the identity's own buffer and the copy
lib/Pipeline.cpp	hoisting after that pass
lib/Conversion/backend/ScheduleIRToDFIR/KTDFLowToDFIR/OperationLowerings.cpp	a copy from a buffer loads first
lib/Transforms/ApplyDevicePatterns.cpp	ktdf.with_precision
lib/Conversion/backend/ScheduleIRToDFIR/KTDFLowToDFIR/LinalgLowering.cpp	minnumf lowers to a min; a compare and a selection lower
lib/Conversion/frontend/KTIRToScheduleIR/KTIRLegalityCheck.cpp	a compare and a selection are accepted in a body
test/Transforms/MapReductionPartials/*	the extra buffer, fill and copy
test/dataflow-scheduler-opt/emit-dfir-pipeline.mlir	the two hoisting passes in the listing

Testing

llvm-lit dataflow-scheduler/test: 191 passed, 4 expected failures, 2 unsupported.
Each of the first three commits was built and run on its own and is green at that
point, so the branch bisects.

linalg-minnumf-lowering.mlir pins the new case. The kernel that could not be
compiled before it was also checked end to end downstream, which is where the error
above came from.

The downstream consumer covers this end to end. Its reduction kernels -- sum, min,
max and a new absolute-max -- now pin the identity as a register's value rather than
as an immediate, two of the four in f32, which is the case that could not be
compiled before. Its suite passes, 28 of 28.

No test here pins the register's value, because that is visible in the generated
program rather than in the IR this repo's tests check.

[KFAFSP]

In these cases it would be nice to return rewriter.notifyMatchFailure(copy_op, "message") to tell our future selves why this pattern can't handle the scenario. This will also be printed during debugging.

[vswagath1989]

Done in 6b6d778 — both bail-outs now say why:

return rewriter.notifyMatchFailure(copy_op, "source is neither a fifo read nor a memref");
return rewriter.notifyMatchFailure(copy_op, "source has no static shape to load as one vector");

[acgatea1]

This seems wrong because both MinimumFOp and MinNumFOp are lowered to VectorChainBinaryOperator::min, but their semantics are slightly different.

[vswagath1989]

You are right that they differ, and I could not settle it from the tree — so I have stated the assumption rather than fixed it, and I would like your call on which way to go.

The two differ only in which operand a NaN takes: minimumf propagates it, minnumf returns the non-NaN side. The unit has one minimum, so mapping both to it is exact only where neither operand is NaN. 6b6d778 says that in the case rather than leaving it implicit.

Two things made me stop short of restricting it:

1. maxnumf and maximumf both map to max already, from [MapReductionPartials , LinalgLowering] Fix maxnumf→max, add absf+maxnumf→abs_max fusion #130. Restricting minnumf alone would leave min and max inconsistent, and the same objection applies to the max pair.
2. I could not find the unit's NaN behaviour documented anywhere in the tree, so I would only be guessing which of the two arith ops the hardware actually implements.

If you know which it is, the clean fix is to lower only the matching op and reject the other at the frontend — for both min and max. Happy to do that here, or to drop the minnumf case again and leave it rejected until the max pair is sorted out too. Which would you prefer?

[acgatea1]

I'm not sure either about what the hardware implements. I think the best thing to do is reject both the minnumf and maxnumf (except for abs_max) cases until this is sorted out. Currently MapReductionPartials already rejects both; we should update LinalgLowering.cpp to do this too. I.e. remove or comment out the MinNumF case and for MaxNumFOp reject if matchAbsMaxOperands returns false.

[KFAFSP]

If this triggers a deeper investigation, I'd like to bring up some more details.

IEEE-754 2008 used to have minNum and maxNum, which were removed in 2019 in favor of minimumNumber, maximumNumber, minimum and maximum. The arith.minnumf and arith.maxnumf operations reference the 2008 operations, which are no longer in the standard.

The old operations are not associative, and leave many details up to the implementation. One of those is how signed zero is handled. Another issue is with NaN propagation, which is not just "does it go through" but also "should signalling NaN be treated differently from quiet NaN". I remember reading that every platform did something different.

Note that there is an RFC for adding the new ops to arith, but since we're not gonna bump I presume, that point is kinda moot. Still, I wouldn't use arith.maxnumf/arith.minnumf for anything new, unless we actually fit that envelope.

If it turns out that our hardware has a specific implementation that we might want the front-end to exploit, we would need a spyreop intrinsic.

[vswagath1989]

Done in 219e078, both together as you suggested:

• The MinNumFOp cases are gone from both element-wise blocks, and MinNumFOp is out of the frontend allow-list — so minnumf is now turned away up front rather than accepted and then failing below.
• MaxNumFOp keeps the abs-of-both path and notifyMatchFailurees when matchAbsMaxOperands returns false, so abs_max still lowers and plain maxnumf does not.

You were right that the other two paths already did this: the reduction chain in lowerReductionGenericOp only accepts maxnumf as part of the three-op absf/absf/maxnumf body, and MapReductionPartials only gives a neutral element for that pattern. So this is the element-wise path catching up rather than a new position.

One test had to move. linalg-maxnumf-lowering.mlir pinned maxnumf alone → binary_operator max, which is now failed to run operation lowerings. It keeps the two abs_max cases; the plain one is a negative test in linalg-maxnumf-plain-rejected.mlir. Suite is 191 passed, 4 expected failures.

[vswagath1989]

That is very useful, thank you — and it settles the direction rather than just the detail. I had been treating this as "which of the two does the hardware do", when the real answer is that the 2008 operations do not pin down enough to be worth mapping at all.

219e078 turns both away: minnumf at the frontend, and maxnumf everywhere except the absf-of-both shape that selects abs_max. The comment on the remaining case says why in your terms — withdrawn in 2019, NaN and signed-zero handling left to the implementation — so the next person does not have to rediscover it.

Agreed on not reaching for these for anything new. If the unit turns out to have a behaviour worth exposing, a spyreop intrinsic is the honest way to say so, since it can carry the exact semantics instead of borrowing an operation that no longer has any.

[acgatea1]

LGTM