[PATCH] D118141: [flang] Upstream partial lowering of EXIT intrinsic

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks @josh.mottley.arm , comments inline!



================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:123-126
+/// Check the operations in \p block for containing a `fir::CallOp` operation
+/// where its name matches \p fctName and the number of arguments is equal to \p
+/// nbArgs. Note that this check only cares if the operation exists, and not the
+/// order in when the operation is called.
----------------
Could you refine this a bit? 

* "Check the operations in \p block for containing a `fir::CallOp` operation where its name matches \p fctName" -> this suggests that the name of `fir::CallOp` should be `fctName`, but in fact it's the function that `fir::CallOp` calls that's matched against `fctName`
* This method will only check the first instance of `fir::CallOpt(fctName)`and exit, right? I think that that's fine, but worth documenting too.


================
Comment at: flang/unittests/Optimizer/Builder/Runtime/RuntimeCallTestBase.h:127-142
+static inline void checkBlockForCallOp(
+    mlir::Block *block, llvm::StringRef fctName, unsigned nbArgs) {
+  EXPECT_TRUE(block);
+  bool opFound = false;
+  for (auto &op : block->getOperations()) {
+    if (auto callOp = mlir::dyn_cast<fir::CallOp>(op)) {
+      mlir::SymbolRefAttr callee = *callOp.callee();
----------------
A bit more context:

* ` EXPECT_TRUE(block);` --> I would move this to where the block is created. In this function I'd use `assert`instead (to verify that this method is used correctly)
* you don't really need `opFound` :)
* everything else is a "nit" - feel free to ignore


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118141