[flang-commits] [PATCH] D141136: [flang] Allow and use fir.rebox in fir.global

[Slava Zakharin via Phabricator via flang-commits]

vzakhari accepted this revision.
vzakhari added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1975
+    // manager inserted a builtin.unrealized_conversion_cast that was inserted
+    // and needs to be removed here.
+    if (isInGlobalOp(rewriter))
----------------
jeanPerier wrote:
> vzakhari wrote:
> > Jean, can you please explain how the `mlir::UnrealizedConversionCastOp` is actually removed?  I see that here we "ignore" it and use its operand instead of the result, but the operation should be still there.  Is it removed somewhere just because it does not have any uses?
> I thought this was trivially some DCE at play before you asked, but it is a bit more subtle. My understanding is the following:
> - mlir::UnrealizedConversionCastOp are added automatically by the dialect translation pass framework to satisfy type MLIR constraints while doing a dialect translation (at [1]).
> - At the end of the conversion pass, the framework tries to get rid of the inserted casts, it succeeds at [2] in this case because the FIR to LLVM IR translation registers a handler  (at [3]) that just uses the builtin.unrealized_conversion_cast operand as what the builtin.unrealized_conversion_cast should be rewritten to (which allows supporting unused  builtin.unrealized_conversion_cast, since it is they can be rewritten to any existing value in the IR with no effect).
> 
> There is also a "reconcile-unrealized-casts" mlir pass to get rid of builtin.unrealized_conversion_cast in some cases [4], but it is not used in FIR pipeline.
> 
> [1]: https://github.com/llvm/llvm-project/blob/0de16fafa57153a70168d69ca8591dbe8489905f/mlir/lib/Transforms/Utils/DialectConversion.cpp#L428
> [2]: https://github.com/llvm/llvm-project/blob/0de16fafa57153a70168d69ca8591dbe8489905f/mlir/lib/Transforms/Utils/DialectConversion.cpp#L2785
> [3]: https://github.com/llvm/llvm-project/blob/0de16fafa57153a70168d69ca8591dbe8489905f/flang/lib/Optimizer/CodeGen/TypeConverter.h#L148
> 
> [4]: https://github.com/llvm/llvm-project/blob/67d0d7ac0acb0665d6a09f61278fbcf51f0114c2/mlir/lib/Conversion/ReconcileUnrealizedCasts/ReconcileUnrealizedCasts.cpp
Thank you for the detailed explanation, Jean!  This looks brittle.  I think it might be worth adding a comment around [3] to mention this box case, so that there is more context when the FIXME notes are being worked on.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141136/new/

https://reviews.llvm.org/D141136