[PATCH] D111796: [fir] Add IfBuilder and utility functions
Andrzej Warzynski via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 14 08:42:41 PDT 2021
awarzynski added a comment.
Thank you @clementval , the tests make it so much easier to review!
================
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();
+ }
----------------
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.
================
Comment at: flang/include/flang/Optimizer/Builder/FIRBuilder.h:67
+ /// Helper class to create if-then-else in a structured way:
+ /// Usage: genIfOp().then([&](){...}).else([&](){...}).end();
+ /// Alternatively, getResults() can be used instead of end() to end the ifOp
----------------
Perhaps I'm missing something here.
================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:16
+public:
+ void SetUp() {
+ fir::KindMapping kindMap(&context);
----------------
> Use override in C++11 to make sure you spelled it correctly.
https://github.com/google/googletest/blob/master/docs/primer.md
Not a blocker.
================
Comment at: flang/unittests/Optimizer/Builder/FIRBuilderTest.cpp:27
+ mlir::MLIRContext context;
+ fir::FirOpBuilder *firBuilder;
+};
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111796/new/
https://reviews.llvm.org/D111796
More information about the llvm-commits
mailing list