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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks for all the answers, Kiran! Accepting as in, all my suggestions are nits.



================
Comment at: flang/lib/Lower/Bridge.cpp:199
+    builder->setInsertionPointToStart(newBlock);
+    if (blockIsUnterminated())
+      builder->setInsertionPointToEnd(newBlock);
----------------
kiranchandramohan wrote:
> 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.
Ah, now I see, thanks! `getFIRBranch` updates the current block. `builder->setInsertionPointToStart` updates the builder. When reading this function I just assumed that only the current block is updated. My bad.

> I can add a comment if that helps. 

It would be nice, yes. Not a blocker though!


================
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
----------------
kiranchandramohan wrote:
> 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.
Thanks for clarifying, Kiran! I totally missed that from the comment. Perhaps  `createEmptyGlobalBlocks` instead of `createEmptyBlocks` would make more sense?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118983