[llvm-commits] [PATCH] Add Foreach Loop

[Jakob Stoklund Olesen]

On Feb 9, 2012, at 12:21 PM, David Greene <dag at cray.com> wrote:

> Here is an updated version of the foreach patch series, squashed
> as requested.  I think I have addressed all of the review feedback.
> Please review.

The syntax in the documentation is still wrong.

> +  // Foreach management.
> +
> +  /// pushLoop - Add a loop to the existing loop context.
> +  ///
> +  void pushLoop(const ForeachLoop &loop) {
> +    Loops.push_back(loop);
> +  }
> +
> +  /// popLoop - Pop a loop from the existing loop context.
> +  ///
> +  void popLoop() {
> +    Loops.pop_back();
> +  }
> +
> +  /// topLoop - Return a reference to the current innermost loop.
> +  ///
> +  ForeachLoop &topLoop() {
> +    assert(!Loops.empty() && "Loop underflow!");
> +    return Loops.back();
> +  }
> +
> +  typedef LoopVector::iterator loop_iterator;
> +
> +  loop_iterator loops_begin() {
> +    return Loops.begin();
> +  }
> +  loop_iterator loops_end() {
> +    return Loops.end();
> +  }
> +  std::size_t loops_size() {
> +    return Loops.size();
> +  }

This all seems excessive for a private variable. Just use Loops directly?

> diff --git old/llvm/test/TableGen/NestedForeach.td new/llvm/test/TableGen/NestedForeach.td
> new file mode 100644
> index 0000000..05e0952
> --- /dev/null
> +++ new/llvm/test/TableGen/NestedForeach.td
> @@ -0,0 +1,74 @@
> +// RUN: llvm-tblgen %s | FileCheck %s
> +// XFAIL: vg_leak
> +
> +class Droid<string series, int release, string model, int patchlevel> {
> +  string Series = series;
> +  int Release = release;
> +  string Model = model;
> +  int Patchlevel = patchlevel;
> +}
> +
> +foreach string S = ["R", "C"] {
> +  foreach int R = [2, 3, 4] {
> +    foreach string M = ["D", "P", "Q"] {
> +      foreach int P = [0, 2, 4] {
> +        def S#R#M#P : Droid<S, R, M, P>;
> +      }
> +    }
> +  }
> +}

Please add a test that has sibling nested loops and refs at different levels.

foreach {
  def
  foreach {
    def
  }
  def
  foreach {
    foreach { … }
    def
  }
}

etc.

Regarding the syntax, I think you should make it parallel with the let keyword:

  foreach string x = […] in { … }

You should also make the braces optional when wrapping a single loop or def, just like let does.

Is it strictly necessary with a type? Let doesn't seem to need it.


/jakob