[PATCH] D111796: [fir] Add IfBuilder and utility functions

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:43-47
+  mlir::Type getIntPtrType() {
+    // TODO: Delay the need of such type until codegen or find a way to use
+    // llvm::DataLayout::getPointerSizeInBits here.
+    return getI64Type();
+  }
----------------
awarzynski wrote:
> IMO this is a bit fragile. I appreciate that we won't be able to use `llvm::DataLayout` for a while, but I fear that we will forget to update this once we can. Maybe we should leave something like `static_assert(sizeof(void*) <= 8)` here? For the sake of well-being of our future selves.
What will this static assert caught? Building the compiler on a platform that has pointers larger than 8 bytes? The check should be about the target and not the host though.

Now there should be a solution for this in MLIR, I'm wondering why it isn't deployed here (and if this API is the right level for exposing this): https://mlir.llvm.org/docs/DataLayout/

I'd say that at least this API will make it easier to find the places to update in the codebase compared to use directly `getI64Type` everywhere...


================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:27
+  mlir::MLIRContext context;
+  fir::FirOpBuilder *firBuilder;
+};
----------------
awarzynski wrote:
> Does this have to be a pointer? If yes, then with e.g. `unique_ptr` you wouldn't have to call `delete`. Not a blocker for me.
I'd rather see a unique_ptr as well here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111796