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#3044 SVE encoder: Add support for sve_int_bin_pred_log enc group. #3078

Merged
merged 10 commits into from
Jul 3, 2018

Conversation

fhahn
Copy link
Contributor

@fhahn fhahn commented Jun 30, 2018

This patch adds a new register type for SVE predicate registers
(P0-P15) and support for the sve_int_bin_pred_log encoding group.

The macros added follow the SVE assembly syntax, where the shared input
and output register Zdn has to be supplied twice.

Issue #3044

This patch adds a new register type for SVE predicate registers
(P0-P15) and support for the sve_int_bin_pred_log encoding group.

The macros added follow the SVE assembly syntax, where the shared input
and output register Zdn has to be supplied twice.

Issue #3044

Change-Id: Ib55b5e1160ab41b9c298b7abbef66a5454ed4384
@fhahn fhahn requested review from derekbruening and egrimley June 30, 2018 21:45
@fhahn
Copy link
Contributor Author

fhahn commented Jul 2, 2018

cc @andreipoe

@@ -909,6 +909,21 @@ encode_opnd_float_reg(int pos, opnd_t opnd, OUT uint *enc_out)
return true;
}

/* p: used to encode an SVE predicate register. */
Copy link
Contributor

@derekbruening derekbruening Jul 2, 2018

Choose a reason for hiding this comment

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

"p:" is confusing: predicate ("p") register?

@@ -1388,6 +1403,21 @@ encode_opnd_extam(uint enc, int opcode, byte *pc, opnd_t opnd, OUT uint *enc_out
return true;
}

/* p10_low: P register at bit position 10; P0-P7 */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: p in comment above but P here

/* -------- SVE bitwise logical operations (predicated) ---------------- */

/**
* Creates a ORR scalable vector instruction.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/a/an/

instr_create_1dst_4src(dc, OP_orr, Zd, Pg, Zd_, Zm, width)

/**
* Creates a EOR scalable vector instruction.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto -- and more below

core/arch/opnd.h Outdated
@@ -272,6 +272,11 @@ enum {
DR_REG_Z20, DR_REG_Z21, DR_REG_Z22, DR_REG_Z23,
DR_REG_Z24, DR_REG_Z25, DR_REG_Z26, DR_REG_Z27,
DR_REG_Z28, DR_REG_Z29, DR_REG_Z30, DR_REG_Z31,

DR_REG_P0, DR_REG_P1, DR_REG_P2, DR_REG_P3,
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks binary compatibility as it changes the value of DR_REG_NZCV, etc. below. (Those DR_REG_Z* are also newly inserted (right?) and also break it.) Please either insert at the end or list this change under the compatibility-breaking part of the changelist in release.dox.

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.

(Maybe best to take a look at the changelist change)

@fhahn
Copy link
Contributor Author

fhahn commented Jul 2, 2018

Thanks Derek. I've addressed the comments. What do you mean with 'changelist change'?

@derekbruening
Copy link
Contributor

What do you mean with 'changelist change'?

If you went the route of breaking compatibility and editing the changelist I just wanted to take a look at that edit before merging.

@@ -79,6 +79,8 @@ const char * const reg_names[] = {
"z8", "z9", "z10", "z11", "z12", "z13", "z14", "z15",
"z16", "z17", "z18", "z19", "z20", "z21", "z22", "z23",
"z24", "z25", "z26", "z27", "z28", "z29", "z30", "z31",
"p0", "p1", "p2", "p3", "p4", "p5", "p6", "p7",
"p8", "p9", "p10", "p11", "p12", "p13", "p14", "p15",
Copy link
Contributor

Choose a reason for hiding this comment

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

This order no longer matches the enum

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done thanks. I've moved everything to the end.

fhahn added 3 commits July 3, 2018 17:05
…e-bit-logic-pred

Change-Id: I20d237be440dd6db7582f7462ddce80ec7fcaf59
Change-Id: I35717046b139418e798333c292bc1c363fa3feea
Change-Id: Ia423c87bd96b29771709c848bfc02e97c7cd06be
@fhahn fhahn merged commit 90ec542 into master Jul 3, 2018
@fhahn fhahn deleted the i3044-sve-bit-logic-pred branch July 3, 2018 17:34
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.

2 participants