[PATCH] D118983: [Flang] Add support for lowering the goto statement

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Thanks @awarzynski for the comments.



================
Comment at: flang/lib/Lower/Bridge.cpp:136
+  Fortran::lower::pft::Evaluation &getEval() {
+    assert(evalPtr);
+    return *evalPtr;
----------------
awarzynski wrote:
> [nit] Could you add a message here?
Will do.


================
Comment at: flang/lib/Lower/Bridge.cpp:198-200
+    builder->setInsertionPointToStart(newBlock);
+    if (blockIsUnterminated())
+      builder->setInsertionPointToEnd(newBlock);
----------------
awarzynski wrote:
> Otherwise you might be setting the insertion point more than once, right?
As mentioned in the previous comment this change will change the block on which the function `blockIsUnterminated` will be called.


================
Comment at: flang/lib/Lower/Bridge.cpp:199
+    builder->setInsertionPointToStart(newBlock);
+    if (blockIsUnterminated())
+      builder->setInsertionPointToEnd(newBlock);
----------------
awarzynski wrote:
> Wouldn't `genFIRBranch(newBlock)` above create a terminator? So that this `if` condition is always `true`?
Note the setting of insertion point to the `newBlock` in the line above. So these two are called for different blocks.
I can add a comment if that helps. I was also a bit confused and had to read again.


================
Comment at: flang/lib/Lower/Bridge.cpp:245-254
+  /// Create global blocks for the current function.  This eliminates the
+  /// distinction between forward and backward targets when generating
+  /// branches.  A block is "global" if it can be the target of a GOTO or
+  /// other source code branch.  A block that can only be targeted by a
+  /// compiler generated branch is "local".  For example, a DO loop preheader
+  /// block containing loop initialization code is global.  A loop header
+  /// block, which is the target of the loop back edge, is local.  Blocks
----------------
awarzynski wrote:
> I don't fully follow this comment. For example, it mentions "global" and "local" blocks, but there's no mention of "global" or "local" in the actual code. Perhaps that's just me?
The comment just says what kind (= global) of blocks are created here. Blocks are either created due to explicit control-flow (goto like statements) or due to implicit control flow (like the back edges of loops).

They are called global because they can be target of other branch instructions, whereas local blocks are targets of implicit branches that are inside a construct.

Note: I also had to read it twice. But not sure whether the above helps or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118983