Attention is currently required from: Felix Singer, Lance Zhao, Nico Huber. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60645 )
Change subject: arch/x86/acpi: Replace Add(a,b,c) with ASL 2.0 syntax ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
In the past, we had patches that covered all changes in a file that didn't change the AML and a separate one for things that did change AML. It's not always wrong when it changes, for instance there are multiple ways to store the result, Store() vs. last argument to an op. IIRC, with the old syntax the compiler translated things literally while with the new syntax it tries to optimize that. Just an example, the compiler might behave differently by now.
That is a good point, I am not sure how much "optimization" IASL applies to the resulting AML.
- medium Per-Method or per-Device changes
This sounds reasonable. I guess I wouldn't mind if the surrounding code would look consistent.
For the piecemeal approach to ASL 1.0 -> 2.0 syntax updates, this has the advantage of keeping at least some consistency
- Small, per-operand changes which are nearly trivial to review
Well, I don't doubt that there are trivial changes. But it doesn't seem trivial to decide which are trivial and which are not. The spec says the new ops act like their C equivalents but it already contradicts itself with the precedence rules. And I have doubts that things like &&, || really do the same that we are used to.
That is a very good point, compound expressions may not have exactly the same AML output, even if the result is the same (or not, I don't know offhand if && and || are lazily evaluated like C...)
There may be more such subtleties. Over all, it seems the ASL 2.0 ops do what the ASL designers *believed* their C equivalents would do :D OTOH, I wouldn't mind keeping per-op changes. Even if we only compare the AML after a dozen changes, the commit message can say so and we'd submit them all at once? The tricky part seems to be to find which hunks result in AML changes and which don't. How the patches that don't result in AML changes are organized doesn't seem to matter that much.
I would hope something like ``` ... Store(Local0, Local1) ``` vs. translates literally into ``` Local1 = Local0 ```
and both have the exact same AML representation.
I guess the compound expressions (or things like lazy evaluations after &&) may prove to be the trickiest parts to update confidently without testing.
Reminds me, I should go through and start updating at least the chrome EC ASL code wholesale to 2.0 syntax, that is easy enough for me to test.