Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#1686 decoder tests: Add some missing OP_vbic_{i16,i32} encodings. #2493

Merged
merged 3 commits into from
Jul 4, 2017

Conversation

egrimley
Copy link
Contributor

@egrimley egrimley commented Jul 3, 2017

Replace some "INVALID" lines in A32_ext_simd8.
Also correct the opcodes in A32_ext_bit19.

Change-Id: I5fcf10fd028da215730a4f2327a317ab1f2a9e98

Replace some "INVALID" lines in A32_ext_simd8.
Also correct the opcodes in A32_ext_bit19.

Change-Id: I5fcf10fd028da215730a4f2327a317ab1f2a9e98
@derekbruening
Copy link
Contributor

If you're working on the missing opcodes for #2465, please assign to yourself to avoid duplicate work -- I was planning to fill in the missing decode table entries and just now was going to assign to me.

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

Are any other encodings known to be missing?

Probably i#2465 can be closed after this, though there are a couple of other matters arising there which might be worth creating an issue for, depending on what you and other people think: DEBUG=intermediate and non-DEBUG ASSERTs/printfs.

@derekbruening
Copy link
Contributor

Please reference #2465 here

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

Shall I just add the following paragraph to the log message?

This fixes the primary cause of the crash reported in i#2465.

Or shall I change the issue number and add "Fixes #2565"?

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

(That would be "i#2465 A32 decoder: Add some missing ..." in the latter case.)

@derekbruening
Copy link
Contributor

I would put it in the title: "i#2465, i#1686: ..." or even demote #1686 to just a xref in the body. This patch is coming directly from #2465. As to whether it fixes #2465 it depends on whether the other issues are separated out or not.

@derekbruening
Copy link
Contributor

I assume you are also fixing the internal consistency error and have a separate patch for that? (It is hard to tell as not everything seems to be posted in the issue.)

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

I don't think I've considered any internal consistency error.

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

Proposed new log message:

i#2465 A32 decoder: Add some missing OP_vbic_{i16,i32} encodings.

Replace some "INVALID" lines in A32_ext_simd8.
Also correct the opcodes in A32_ext_bit19.

Xref #1686.

Fixes #2465

@derekbruening
Copy link
Contributor

Without fixing the internal consistency it doesn't "fixes #2465": it only fixes 1 of the 6 issues. I guess 4 of them are covered by other issues but the internal consistency remains.

Updated log message:

i#2465 A32 decoder: Add some missing SIMD encodings.

Replace some "INVALID" lines in A32_ext_simd8 with missing encodings:
OP_vbic_{i16,i32} and OP_vmov_f32.

Also correct the opcodes in A32_ext_bit19.

Xref #1686.

Change-Id: I150ddc01484a7cbf5a866d8ab40940ebe7a9311c
@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

I think the "Encode/decode inconsistency" would be a separate pull request. It doesn't involve SIMD, I don't know whether it involves the decoder, and I don't yet have the test case to reproduce it.

@egrimley
Copy link
Contributor Author

egrimley commented Jul 3, 2017

AppVeyor failure is #2246.

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that in general changing these tables requires re-running the encode chain script and the instr create script (not fully automated, they are manually run with manually inserted output for the macros). In this case we don't expect either script to change anything as these are all dup entries but we don't want to break them and they may catch something we humans can't easily see (esp w/o as many arm tests as we'd like).

The title is now too narrow.

Thank you for fixing this (though next time please self-assign the issue prior to starting coding).

@@ -298,11 +298,11 @@ const instr_info_t A32_ext_bit7[][2] = {
*/
const instr_info_t A32_ext_bit19[][2] = {
{ /* 0 */
{EXT_SIMD8, 0xf2800000, "(ext simd8 0)", xx, xx, xx, xx, xx, no, x, 0},
{EXT_SIMD5, 0xf2880000, "(ext simd5 0)", xx, xx, xx, xx, xx, no, x, 0},
{EXT_SIMD8, 0xf2800010, "(ext simd8 0)", xx, xx, xx, xx, xx, no, x, 0},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it looks like all children have bit 4 set, but it doesn't really matter at this split point: just informative, nothing reads it.

Change-Id: I2cf04ec6945ab8154d7cdb6855161b5a124745a6
@egrimley egrimley merged commit 32374b2 into master Jul 4, 2017
@egrimley egrimley deleted the i1686-vbic branch July 4, 2017 08:43
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.

3 participants