[aes] Hierarchical paths in AES RTL won't synthesize in DC

[msfschaffner]

Hey @vogelpi - I just realized that we have added hierarchical paths in the AES RTL as part of the latest AES update:

opentitan/hw/ip/aes/rtl/aes_sbox_dom.sv

Lines 681 to 684 in 3cc8d5a

	assign a_gamma1_q = u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_x_q;
	assign a_gamma0_q = u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_y_q;
	assign b_gamma1_q = u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_x_q;
	assign b_gamma0_q = u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_y_q;

This is not synthesizable and should only be used in TB code.
We need to fix this since otherwise DC errors out.

I can try and fix this tomorrow - but if anyone can take a look in the meantime that would be great.

[vogelpi]

This syntax worked fine both in Yosys and Vivado. But I understand that it needs to be fixed. I'll have a look and for sure will have a PR ready by tomorrow morning. Sorry for causing this issue!

[tjaychen]

sorry for missing this during the earlier review Pirmin.

…

On Wed, Jan 19, 2022 at 2:21 PM Pirmin Vogel ***@***.***> wrote: This syntax worked fine both in Yosys and Vivado. But I understand that it needs to be fixed. I'll have a look and for sure will have a PR ready by tomorrow morning. Sorry for causing this issue! — Reply to this email directly, view it on GitHub <#10180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2RSQO25XEPIEVB6M66VTUW42PLANCNFSM5MI65FAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were assigned.Message ID: ***@***.***>

[msfschaffner]

Hey I missed it as well ;).

I drafted a fix here: #10206

[vogelpi]

This is kind of embarrassing: it turned out that Yosys also doesn't like the hierarchical reference. The resulting netlist (I've formally verified) was non functional :-(
On the new, correct netlist, formal masking verification takes much longer. At least we know that Vivado did the right thing (otherwise CI would have failed) and we also didn't see leakage in the FPGA experiments.

[vogelpi]

Hey I missed it as well ;).

I drafted a fix here: #10206

Sorry Michael, I should have pinged you about the PR I did a couple of hours before yours: #10202

[tjaychen]

This is kind of embarrassing: it turned out that Yosys also doesn't like the hierarchical reference. The resulting netlist (I've formally verified) was non functional :-( On the new, correct netlist, formal masking verification takes much longer. At least we know that Vivado did the right thing (otherwise CI would have failed) and we also didn't see leakage in the FPGA experiments.

this is sort of a side topic. But does this mean we should file a bug on Yosys? If it actually doesn't like it, I feel like the flow should have error'd out instead of continuing to do something...weird.

[msfschaffner]

Hey I missed it as well ;).
I drafted a fix here: #10206

Sorry Michael, I should have pinged you about the PR I did a couple of hours before yours: #10202

No worries, I closed my PR.

[msfschaffner]

This is kind of embarrassing: it turned out that Yosys also doesn't like the hierarchical reference. The resulting netlist (I've formally verified) was non functional :-( On the new, correct netlist, formal masking verification takes much longer. At least we know that Vivado did the right thing (otherwise CI would have failed) and we also didn't see leakage in the FPGA experiments.

this is sort of a side topic. But does this mean we should file a bug on Yosys? If it actually doesn't like it, I feel like the flow should have error'd out instead of continuing to do something...weird.

We probably should, yes. @vogelpi can you take care of this?

[vogelpi]

After checking the sv2v output I can confirm it's a Yosys issue (and not our sv2v + Yosys flow). Checking the Yosys repository, it seems this is known (see YosysHQ/yosys#2266 and YosysHQ/yosys#647) and it produces at least warnings in the synthesis log:

syn_out/aes_sbox/generated/aes_sbox_dom.v:628: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_x_q' is implicitly declared.
syn_out/aes_sbox/generated/aes_sbox_dom.v:629: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_y_q' is implicitly declared.
syn_out/aes_sbox/generated/aes_sbox_dom.v:630: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_x_q' is implicitly declared.
syn_out/aes_sbox/generated/aes_sbox_dom.v:631: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_y_q' is implicitly declared.

The problem isn't easy to solve but @rswarbrick seems to have prepared a draft PR already that moves things in the right direction.

I will file a PR to our coding style guide to discourage usage of hierarchical references for code that is meant to be synthesized.

[tjaychen]

thank you Pirmin and Rupert.

…

On Fri, Jan 21, 2022 at 1:35 AM Pirmin Vogel ***@***.***> wrote: After checking the sv2v output I can confirm it's a Yosys issue (and not our sv2v + Yosys flow). Checking the Yosys repository, it seems this is known (see YosysHQ/yosys#2266 <YosysHQ/yosys#2266> and YosysHQ/yosys#647 <YosysHQ/yosys#647>) and it produces at least warnings in the synthesis log: syn_out/aes_sbox/generated/aes_sbox_dom.v:628: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_x_q' is implicitly declared. syn_out/aes_sbox/generated/aes_sbox_dom.v:629: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.a_y_q' is implicitly declared. syn_out/aes_sbox/generated/aes_sbox_dom.v:630: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_x_q' is implicitly declared. syn_out/aes_sbox/generated/aes_sbox_dom.v:631: Warning: Identifier `\u_aes_dom_mul_gamma1_gamma0.gen_pipeline.b_y_q' is implicitly declared. The problem isn't easy to solve but @rswarbrick <https://github.com/rswarbrick> seems to have prepared a draft PR already that moves things in the right direction. I will file a PR to our coding style guide to discourage usage of hierarchical references for code that is meant to be synthesized. — Reply to this email directly, view it on GitHub <#10180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2RSVPV4XVFIJ4EEC4353UXESGNANCNFSM5MI65FAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were assigned.Message ID: ***@***.***>