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

[Valentin Clement via Phabricator via llvm-commits]

clementval 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();
+  }
----------------
mehdi_amini wrote:
> 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...
The code is probably older than the introduction of the API you mention. I'll check with other whether we can use this in replacement. 
As a side note, there is a lot of code to be upstream and probably some of the code is not up to date with the latest MLIR advancement and best practices. We try to update that as we go. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111796