[flang-commits] [PATCH] D144743: [flang] Fix lowering of optional char proc args

[Leandro Lupori via Phabricator via flang-commits]

luporl added inline comments.


================
Comment at: flang/lib/Optimizer/Builder/FIRBuilder.cpp:623-627
+  auto boxTy =
+      argTy.cast<mlir::TupleType>().getType(0).cast<fir::BoxProcType>();
+  mlir::Type funcTy = boxTy.getEleTy();
+  mlir::Value funcAddr = create<fir::AbsentOp>(loc, funcTy);
+  auto boxProc = create<fir::EmboxProcOp>(loc, boxTy, funcAddr);
----------------
jeanPerier wrote:
> luporl wrote:
> > jeanPerier wrote:
> > > Thinking this through a bit more, doesn't `auto boxProc = create<fir::AbsentOp>(loc, argTy.cast<mlir::TupleType>().getType(0));` works an save the EmboxProc at that level?
> > > I think AbsentOp should work properly on BoxProcType (otherwise, there would also be a bug for optional non character dummy procedures).
> > > 
> > > It will probably also make the extension of this code to optional character pointer procedure easier when they arrive (nothing to do other than updating the `isCharacterProcedureTuple` to detect them).
> > I've tried this and it works (assembly seems correct and test program runs without errors).
> > 
> > I'm just a bit worried about the generated FIR, that has an unconditional `fir.box_addr` on the absent BoxProc, in the `if (present(arg))` part. Is `fir.box_addr` guaranteed to work with `fir.absent` BoxProcs?
> > Is fir.box_addr guaranteed to work with fir.absent BoxProcs?
> 
> Yes, it's fine. I agree it is not supper nice to do this box_addr + embox_proc. It is a bit useless an noisy, but is not related to your patch.
> 
> The box_addr is probably generated here: https://github.com/llvm/llvm-project/blob/cedfd2721e3492e5ab0ea86d24d8027846687c27/flang/lib/Lower/ConvertProcedureDesignator.cpp#L63, but it would need to be tested that the box_addr part can be skipped (that the code in lowering is OK with getting a fir.boxproc), so I would not try to change this in this patch.
> 
> 
Ok, thanks for the detailed explanation. Then I'll go ahead with this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144743