Another case of problems with Vec.toSeq

[Øyvind Harboe]

Why doesn't Vec.toSeq return Seq[T]?

https://scastie.scala-lang.org/c3WEJvlvRKGmWHaBYgGaDw

import chisel3._
import chisel3.util.experimental.BoringUtils

class Foo extends Module {
  val array = IO(Input(Vec(3, Vec(3, UInt(3.W)))))
  val total = IO(Output(UInt()))

  // doesn't work  
  // total := array.reduce(_ ++ _).reduce(_ + _)

   total := array.map{s =>
     val l: Seq[UInt] = s.toSeq
                     l}.reduce(_ ++ _).reduce(_ + _)
}

println(getVerilogString(new Foo()))

[Kamyar Mohajerani]

This a due to changes made in Scala library 2.13. 

https://docs.scala-lang.org/overviews/core/collections-migration-213.html

it worked with 2.12:

https://scastie.scala-lang.org/eX6eVAtTRKOl9NK81RFLfw

but toSeq is now inherited from scala.collection.immutable.IndexedSeq[A]

https://github.com/chipsalliance/chisel3/blob/042be4b2d8a846d12c5c5dfd64f10b5caee93125/core/src/main/scala/chisel3/Aggregate.scala#L817

I think it should be possible to just change that to  scala.collection.IndexedSeq, without causing source or binary incompatibility.

-Kamyar

--
You received this message because you are subscribed to the Google Groups "chisel-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chisel-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/chisel-users/5b8bc84e-759b-422d-932f-1fd0877610edn%40googlegroups.com.

[Øyvind Harboe]

I gave it a go, but it fails to compile still...

https://github.com/chipsalliance/chisel3/pull/2360

[Øyvind Harboe]

Surprising:

  // doesn't work
  //total := array.map(_.toSeq).reduce(_ ++ _).reduce(_ + _)

  // works
  total := array.map(_.toIndexedSeq).reduce(_ ++ _).reduce(_ + _)

https://scastie.scala-lang.org/2kZzHFJzTmSaIhTt5bHKiw

[Jack Koenig]

I find this issue incredibly puzzling. I don't think https://github.com/chipsalliance/chisel3/pull/2360 is right, but I really don't understand what the issue is. It should infer the correct upcast to `IndexedSeq`...

Anyway, the usual technique of adding type parameters where they are usually inferred resolves this for me, ie.

total := array.reduce[Seq[UInt]](_ ++ _).reduce(_ + _)

I have no idea why the compiler is not figuring this out for us though.

I'm assuming your actual use case is more complex, but I will note that `.reduce(_ ++ _)` on any Seq is better as just a flatten, eg.

total := array.flatten.reduce(_ + _)

--
You received this message because you are subscribed to the Google Groups "chisel-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chisel-users...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/chisel-users/54d707e1-86d3-45ee-938a-95a8a88e29aen%40googlegroups.com.

[Øyvind Harboe]

Thanks!

I have closed the PR. Before the issues with the Scala compiler/type checking is fully understood, there's no point in trying to change Chisel :-)

On Thursday, January 27, 2022 at 6:05:08 AM UTC+1 Jack Koenig wrote:

I find this issue incredibly puzzling. I don't think https://github.com/chipsalliance/chisel3/pull/2360 is right, but I really don't understand what the issue is. It should infer the correct upcast to `IndexedSeq`...

Anyway, the usual technique of adding type parameters where they are usually inferred resolves this for me, ie.

total := array.reduce[Seq[UInt]](_ ++ _).reduce(_ + _)

I have no idea why the compiler is not figuring this out for us though.

I'm assuming your actual use case is more complex,

Yes. This was just the smallest example I could make of the problem I was seeing.