Skip to content

Revert #2496 #2515

Closed
sequencer wants to merge 1 commit intomasterfrom
fix_2496
Closed

Revert #2496 #2515
sequencer wants to merge 1 commit intomasterfrom
fix_2496

Conversation

@sequencer
Copy link
Copy Markdown
Member

@sequencer sequencer commented May 3, 2022

#2496 introduced a bug which was spotted by my daily CI in
https://github.com/sequencer/playground/runs/6243944547
An MVP should be:

RegEnable(next = true.B, init = true.B, enable = true.B)

won't compile, and Scala complains:

macro applications do not support named and/or default arguments

This was should blame to Scala Compiler in https://github.com/scala/scala/blob/21a56430f9902b5b8b81535fd76371caf23c0d10/src/compiler/scala/tools/nsc/typechecker/ContextErrors.scala#L630
which seems to be a limitation to Scala compiler(both 2.12 and 2.13) that the function which used macro cannot use default or named arguments.

Thus #2496 introduced a breaking change to related function calls.

There exists three options to this:

  1. Reverting Utils: add source locators #2496 to make downstream happy(keep compatibility but the detail source info is lost)
  2. Regard this as a breaking change, user should bear with it.(we should add this to release note)
  3. Refactoring SourceInfo and CompileOption macro to Scala Plugin, put it earlier than macro.(refactor a lot but user will be happier)

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • bug fix

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (Bug fix: 3.4.x, [small] API extension: 3.5.x, API modification or big change: 3.6.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@sequencer sequencer marked this pull request as draft May 3, 2022 19:26
@sequencer sequencer requested review from chick and mwachs5 May 3, 2022 19:57
@sequencer sequencer marked this pull request as ready for review May 3, 2022 19:57
@sequencer sequencer changed the title Fix for 2496 Revert #2496 May 3, 2022
@jackkoenig
Copy link
Copy Markdown
Contributor

There might be a 4th option, we could maybe use @deprecatedName to sort of deprecate the use of by-name parameters on these methods.

We really want to add source locators if at all possible but if we can avoid a hard breakage (without a deprecation step) that would definitely be ideal.

@ekiwi
Copy link
Copy Markdown
Contributor

ekiwi commented May 13, 2022

I am curious to hear your thoughts on this @jackkoenig : How hard / how useful would it be to implement source locator propagation through a compiler plugin instead of using macros?

@sequencer
Copy link
Copy Markdown
Member Author

@deprecatedName sounds to be a great idea!

@sequencer sequencer closed this May 13, 2022
@mwachs5
Copy link
Copy Markdown
Contributor

mwachs5 commented May 13, 2022

wait are we saying that the change to adding source locators means we can't pass named arguments to RegNext and friends? That seems sad in general...

@sequencer
Copy link
Copy Markdown
Member Author

Yes, it seems that Scala compiler doesn't like this, I have seen two downstreams beaten by this:(
chipsalliance/rocket-chip#2986
sifive/sifive-blocks#177

@jackkoenig
Copy link
Copy Markdown
Contributor

I am curious to hear your thoughts on this @jackkoenig : How hard / how useful would it be to implement source locator propagation through a compiler plugin instead of using macros?

All compiler plugin stuff is difficult, we'd essentially be reimplementing a subset of what def macros do. It's probably doable, but I'm not actually convinced that named arguments would work if we did it in a plugin. Macros paradise was implemented as a plugin prior to Scala 2.13 and it has never supported named arguments so I suspect there's some fundamental limitation here.

@jackkoenig
Copy link
Copy Markdown
Contributor

wait are we saying that the change to adding source locators means we can't pass named arguments to RegNext and friends? That seems sad in general...

@mwachs5 - Yes, it is a really annoying limitation but it is why Chisel3 moved to things like UInt(8.W) instead of UInt(width = 8) and RegInit(0.U) instead of Reg(init = 0.U). It's a super annoying limitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants