[all-commits] [llvm/llvm-project] acc159: [mlir][Transforms] Dialect conversion: Fix missing...

[Matthias Springer via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: acc159aea1e641e3694ab8fe5faa231788077011
      https://github.com/llvm/llvm-project/commit/acc159aea1e641e3694ab8fe5faa231788077011
  Author: Matthias Springer <me at m-sp.org>
  Date:   2024-07-15 (Mon, 15 Jul 2024)

  Changed paths:
    M mlir/docs/DialectConversion.md
    M mlir/include/mlir/Dialect/SCF/TransformOps/SCFTransformOps.td
    M mlir/include/mlir/Transforms/DialectConversion.h
    M mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
    M mlir/lib/Dialect/SCF/TransformOps/CMakeLists.txt
    M mlir/lib/Dialect/SCF/TransformOps/SCFTransformOps.cpp
    M mlir/lib/Transforms/Utils/DialectConversion.cpp
    M mlir/test/Conversion/FuncToLLVM/func-memref-return.mlir
    A mlir/test/Transforms/test-block-legalization.mlir

  Log Message:
  -----------
  [mlir][Transforms] Dialect conversion: Fix missing source materialization (#97903)

This commit fixes a bug in the dialect conversion. During a 1:N
signature conversion, the dialect conversion did not insert a cast back
to the original block argument type, producing invalid IR.

See `test-block-legalization.mlir`: Without this commit, the operand
type of the op changes because an `unrealized_conversion_cast` is
missing:
```
"test.consumer_of_complex"(%v) : (!llvm.struct<(f64, f64)>) -> ()
```

To implement this fix, it was necessary to change the meaning of
argument materializations. An argument materialization now maps from the
new block argument types to the original block argument type. (It now
behaves almost like a source materialization.) This also addresses a
`FIXME` in the code base:
```
// FIXME: The current argument materialization hook expects the original
// output type, even though it doesn't use that as the actual output type
// of the generated IR. The output type is just used as an indicator of
// the type of materialization to do. This behavior is really awkward in
// that it diverges from the behavior of the other hooks, and can be
// easily misunderstood. We should clean up the argument hooks to better
// represent the desired invariants we actually care about.
```

It is no longer necessary to distinguish between the "output type" and
the "original output type".

Most type converter are already written according to the new API. (Most
implementations use the same conversion functions as for source
materializations.) One exception is the MemRef-to-LLVM type converter,
which materialized an `!llvm.struct` based on the elements of a memref
descriptor. It still does that, but casts the `!llvm.struct` back to the
original memref type. The dialect conversion inserts a target
materialization (to `!llvm.struct`) which cancels out with the other
cast.

This commit also fixes a bug in `computeNecessaryMaterializations`. The
implementation did not account for the possibility that a value was
replaced multiple times. E.g., replace `a` by `b`, then `b` by `c`.

This commit also adds a transform dialect op to populate SCF-to-CF
patterns. This transform op was needed to write a test case. The bug
described here appears only during a complex interplay of 1:N signature
conversions and op replacements. (I was not able to trigger it with ops
and patterns from the `test` dialect without duplicating the `scf.if`
pattern.)

Note for LLVM integration: Make sure that all
`addArgument/Source/TargetMaterialization` functions produce an SSA of
the specified type.

Depends on #98743.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications