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

8259822: [PPC64] Support the prefixed instruction format added in POWER10 #2095

Closed
wants to merge 11 commits into from
@@ -1387,9 +1387,6 @@ class Assembler : public AbstractAssembler {
inline void pla( Register d, long si34);
inline void pla( Register d, Register a, long si34);
inline void psubi(Register d, Register a, long si34);
// Generate a prefixed or non-prefixed add immediate instruction based on si34 value
inline void paddi_or_addi(Register d, Register a, long si34);
inline void pli_or_li(Register d, long si34);

private:
inline void addi_r0ok( Register d, Register a, int si16);
@@ -1521,12 +1518,9 @@ class Assembler : public AbstractAssembler {
static bool is_paddi_suffix(int x) {
return PADDI_SUFFIX_OPCODE == (x & PADDI_SUFFIX_OPCODE_MASK);
}
static bool is_pli_prefix(int x) {
return is_paddi_prefix(x) && inv_r_eo(x) == 0;
}
static bool is_pli_suffix(int x) {
return is_paddi_suffix(x) && inv_ra_field(x) == 0;
}
// This function can skip a nop for 64-byte alignment and check the next word for a prefix.
// Since the alignement nop is uncommon case, we will keep callers of this function simple,
// as they which are often assertions and complex if-statement.
static int is_paddi(const int* p, bool is_pli = false) {
int32_t* p_inst = (int32_t*)p;

@@ -1540,7 +1534,6 @@ class Assembler : public AbstractAssembler {
return false;
}
}

This comment has been minimized.

@TheRealMDoerr

TheRealMDoerr Mar 24, 2021
Contributor

I can't see any usage of it. As Corey stated, unused code should better get removed because it doesn't get tested and may break unless you're planning to use it in the near future or need it for debugging purposes.

static bool is_pli(const int* p) { return is_paddi(p, true); }
static bool is_mtctr(int x) {
return MTCTR_OPCODE == (x & MTCTR_OPCODE_MASK);
}
@@ -154,14 +154,6 @@ inline void Assembler::paddi_r0ok( Register d, Register a, long si34, bool r =
emit_int32( PADDI_SUFFIX_OPCODE | rt(d) | ra(a) | d1_eo(si34));
}

inline void Assembler::paddi_or_addi(Register d, Register a, long si34) {
if (is_simm(si34, 16)) {
Assembler::addi(d, a, (int)si34);
} else {
Assembler::paddi(d, a, si34);
}
}

// Fixed-Point Arithmetic Instructions with Overflow detection
inline void Assembler::addo( Register d, Register a, Register b) { emit_int32(ADD_OPCODE | rt(d) | ra(a) | rb(b) | oe(1) | rc(0)); }
inline void Assembler::addo_( Register d, Register a, Register b) { emit_int32(ADD_OPCODE | rt(d) | ra(a) | rb(b) | oe(1) | rc(1)); }
@@ -206,14 +198,6 @@ inline void Assembler::pla( Register d, long si34) { Assembler::pad
inline void Assembler::pla( Register d, Register a, long si34) { Assembler::paddi( d, a, si34, false); }
inline void Assembler::psubi(Register d, Register a, long si34) { Assembler::paddi( d, a, -si34, false); }

inline void Assembler::pli_or_li(Register d, long si34) {
if (is_simm(si34, 16)) {
Assembler::li( d, (int)si34);
} else {
Assembler::pli(d, si34);
}
}

// PPC 1, section 3.3.9, Fixed-Point Compare Instructions
inline void Assembler::cmpi( ConditionRegister f, int l, Register a, int si16) { emit_int32( CMPI_OPCODE | bf(f) | l10(l) | ra(a) | simm(si16,16)); }
inline void Assembler::cmp( ConditionRegister f, int l, Register a, Register b) { emit_int32( CMP_OPCODE | bf(f) | l10(l) | ra(a) | rb(b)); }
@@ -5836,6 +5836,9 @@ instruct loadConI32_lo16(iRegIdst dst, iRegIsrc src1, immI16 src2) %{

instruct loadConI32(iRegIdst dst, immI32 src) %{
match(Set dst src);
// This macro is valid only in Power 10 and up, but adding the following predicate here
// caused build error. So we comment it out for now.

This comment has been minimized.

@CoreyAshford

CoreyAshford Mar 24, 2021

small grammar issue:

// caused a build error , so we comment it out for now.

// predicate(PowerArchitecturePPC64 >= 10);
ins_cost(DEFAULT_COST+1);

format %{ "(nop if crossing a 64-byte boundary)\n\t"
@@ -5921,6 +5924,9 @@ instruct loadConL32_Ex(iRegLdst dst, immL32 src) %{
// Load 34-bit long constant using prefixed addi. No constant pool entries required.
instruct loadConL34(iRegLdst dst, immL34 src) %{
match(Set dst src);
// This macro is valid only in Power 10 and up, but adding the following predicate here
// caused build error. So we comment it out for now.

This comment has been minimized.

@CoreyAshford

CoreyAshford Mar 24, 2021

// caused a build error , so we comment it out for now.

This comment has been minimized.

@TheRealMDoerr

TheRealMDoerr Mar 25, 2021
Contributor

// caused a build error , so we comment it out for now.

Better would be, "so we only check it in the operand".

// predicate(PowerArchitecturePPC64 >= 10);
ins_cost(DEFAULT_COST+1);

This comment has been minimized.

@CoreyAshford

CoreyAshford Jan 21, 2021

There's no predicate for >= POWER10. I can see how this works because of the immL34 operand having its own predicate, but in later instructs, e.g. addL_reg_imm34 even though the operand is immI32, you still add the explicit predicate.

I'd rather there be an explicit POWER10 predicate in this instruct.

This comment has been minimized.

@kazunoriogata

kazunoriogata Jan 26, 2021
Author

If predicate is added, adlc fails with an error message: "Syntax Error: :ADLC does not support instruction chain rules with predicates" I think addL_reg_imm34 allows predicate because it is not called from other rules. Is it better to leave some comments? (BTW, immI32 is only for POWER10 or higher. POWER9 version uses immI16 or immI16hi.)

This comment has been minimized.

@CoreyAshford

CoreyAshford Jan 27, 2021

Hmm, I'm confused. I don't see any other reference to loadConL34 in ppc.ad.

This comment has been minimized.

@CoreyAshford

CoreyAshford Feb 1, 2021

I wish we knew why this predicate is causing an issue, but I guess it's not important because the operand type provides sufficient limiting of the instruct.

This comment has been minimized.

@TheRealMDoerr

TheRealMDoerr Mar 23, 2021
Contributor

Ok, I think we can't use a predicate for Set dst src. It's fine to have it in the operand.
Would be nice to describe this in the comment.


format %{ "(nop if crossing 64byte boundary)\n\t"
ProTip! Use n and p to navigate between commits in a pull request.