search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

add bit into AllStubInner for TP Disks #230

Closed jasonfan393 closed 1 year ago

jasonfan393 commented 1 year ago

PR description:

PR records an extra bit into disk allStubInner memories test vectors. This bit is used by the TP in HLS to differentiate the postive and negative disks.

corresponds to firmware_hls PR (280)

this is a copy of closed PR: 229

bryates commented 1 year ago

It looks like the test vectors created from this PR

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

causes a mismatch in the MP. @jasonfan393 and @aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cc?

aryd commented 1 year ago

PR 280 successfully ran the CI before being merged and also in my local tests. I’m not sure what the issue might be. I’m in meetings and can not look carefully right now.

-Anders

Anders Ryd @.**@.>

On Aug 7, 2023, at 6:16 PM, Brent R. Yates @.**@.>> wrote:

It looks like the test vectors created from this PR

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

case a mismatch in the MP. @jasonfan393https://github.com/jasonfan393 and @arydhttps://github.com/aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cchttp://Stub.cc?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/230#issuecomment-1668179160, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPUGEOJBHMIVA7H4T3XUEIGPANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

aryd commented 1 year ago

Brent,

I did not read your email carefully - you are asking about the CMSSW PR? but I was confused because:

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

this is used in the firmware-hls PR #280. I now see that the MP did fail, but the overall CI passed. I’ll try to look at this later.

-Anders

Anders Ryd @.**@.>

On Aug 7, 2023, at 6:16 PM, Brent R. Yates @.**@.>> wrote:

It looks like the test vectors created from this PR

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

case a mismatch in the MP. @jasonfan393https://github.com/jasonfan393 and @arydhttps://github.com/aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cchttp://Stub.cc?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/230#issuecomment-1668179160, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPUGEOJBHMIVA7H4T3XUEIGPANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

aryd commented 1 year ago

Brent,

my previous emails were a bit confused as I was multitasking… I did not connect that the problem was in the MP… I read it as TP…

I ran the MP standalone and tried all the different Layers and Disks that are in the script_MP.tcl file. There is only one single differences in L2 where the HLS code picks up one extra match. So I don’t think that there is any significant change in the test vectors, but that we somehow had different events that tripped some outstanding difference between the HLS and emulation.

-Anders

Anders Ryd @.**@.>

On Aug 7, 2023, at 7:25 PM, Anders Ryd @.**@.>> wrote:

Brent,

I did not read your email carefully - you are asking about the CMSSW PR? but I was confused because:

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

this is used in the firmware-hls PR #280. I now see that the MP did fail, but the overall CI passed. I’ll try to look at this later.

-Anders

Anders Ryd @.**@.>

On Aug 7, 2023, at 6:16 PM, Brent R. Yates @.**@.>> wrote:

It looks like the test vectors created from this PR

https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz

case a mismatch in the MP. @jasonfan393https://github.com/jasonfan393 and @arydhttps://github.com/aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cchttp://stub.cc/?

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/230#issuecomment-1668179160, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPUGEOJBHMIVA7H4T3XUEIGPANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

bryates commented 1 year ago

Brent, my previous emails were a bit confused as I was multitasking… I did not connect that the problem was in the MP… I read it as TP… I ran the MP standalone and tried all the different Layers and Disks that are in the script_MP.tcl file. There is only one single differences in L2 where the HLS code picks up one extra match. So I don’t think that there is any significant change in the test vectors, but that we somehow had different events that tripped some outstanding difference between the HLS and emulation. -Anders Anders Ryd @.**@.> On Aug 7, 2023, at 7:25 PM, Anders Ryd @.**@.>> wrote: Brent, I did not read your email carefully - you are asking about the CMSSW PR? but I was confused because: https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz this is used in the firmware-hls PR #280. I now see that the MP did fail, but the overall CI passed. I’ll try to look at this later. -Anders Anders Ryd @.**@.> On Aug 7, 2023, at 6:16 PM, Brent R. Yates @.**@.>> wrote: It looks like the test vectors created from this PR https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz case a mismatch in the MP. @jasonfan393https://github.com/jasonfan393 and @arydhttps://github.com/aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cchttp://stub.cc/? — Reply to this email directly, view it on GitHub<#230 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPUGEOJBHMIVA7H4T3XUEIGPANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

On top of the 1 extra, there are also some mismatches, possibly due to HLS picking up extras that should be overwritten, though I haven't dug into it yet to confirm whether they should be overwritten.

aryd commented 1 year ago

You mean:

Event: 59 FullMatch 0: index reference computed 0 0x42114918101f6 0x420137b5f81f4 <=== INCONSISTENT 1 0x442130160f1da 0x42114918101f6 <=== INCONSISTENT 2 0x44410609f7007 0x442130160f1da <=== INCONSISTENT 3 0x44493a26051d5 0x44410609f7007 <=== INCONSISTENT 4 0x0 0x44493a26051d5 <=== EXTRA FullMatch 1:

This is one extra match "0x420137b5f81f4 “ at the start and then the other matches are listed as inconsistent.

-Anders

Anders Ryd @.**@.>

On Aug 7, 2023, at 8:24 PM, Brent R. Yates @.**@.>> wrote:

Brent, my previous emails were a bit confused as I was multitasking… I did not connect that the problem was in the MP… I read it as TP… I ran the MP standalone and tried all the different Layers and Disks that are in the script_MP.tcl file. There is only one single differences in L2 where the HLS code picks up one extra match. So I don’t think that there is any significant change in the test vectors, but that we somehow had different events that tripped some outstanding difference between the HLS and emulation. -Anders Anders Ryd @.@.> On Aug 7, 2023, at 7:25 PM, Anders Ryd @.@.>> wrote: Brent, I did not read your email carefully - you are asking about the CMSSW PR? but I was confused because: https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz this is used in the firmware-hls PR cms-sw#280https://github.com/cms-sw/cmssw/pull/280. I now see that the MP did fail, but the overall CI passed. I’ll try to look at this later. -Anders Anders Ryd @.@.> On Aug 7, 2023, at 6:16 PM, Brent R. Yates @.@.>> wrote: It looks like the test vectors created from this PR https://cernbox.cern.ch/remote.php/dav/public-files/G0AQPtbx74QMKQh/MemPrints.tar.gz case a mismatch in the MP. @jasonfan393https://github.com/jasonfan393https://github.com/jasonfan393 and @arydhttps://github.com/arydhttps://github.com/aryd does anything need to be changed in the MP (at least on the emulation side) to handle the changes in Stub.h and Stub.cchttp://Stub.cchttp://stub.cc/? — Reply to this email directly, view it on GitHub<#230 (comment)https://github.com/cms-L1TK/cmssw/pull/230#issuecomment-1668179160>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTPUGEOJBHMIVA7H4T3XUEIGPANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

On top of the 1 extra, there are also some mismatches, possibly due to HLS picking up extras that should be overwritten, though I haven't dug into it yet to confirm whether they should be overwritten.

— Reply to this email directly, view it on GitHubhttps://github.com/cms-L1TK/cmssw/pull/230#issuecomment-1668383186, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABI4LTOVIJRPQEQG7YEK6YLXUEXERANCNFSM6AAAAAA2MD77TM. You are receiving this because you were mentioned.Message ID: @.***>

skinnari commented 1 year ago

as discussed during L1TK meeting on Aug 8, this is good to be merged.