Commit 36846b26 by Richard Henderson Committed by Richard Henderson

mn10300: Cleanup legitimate addresses

Allow REG+REG and POST_MODIFY addressing for AM33.  Fix AM33 base and
index register classes.  Remove a bunch of register class combinations
that aren't really useful after this cleanup.

From-SVN: r169006
parent 8b119bb6
2011-01-19 Richard Henderson <rth@redhat.com> 2011-01-19 Richard Henderson <rth@redhat.com>
* config/mn10300/mn10300.c (mn10300_print_operand_address): Handle
POST_MODIFY.
(mn10300_secondary_reload): Tidy combination reload classes.
(mn10300_legitimate_address_p): Allow post-modify and reg+reg
addresses for AM33. Allow symbolic offsets for reg+imm.
(mn10300_regno_in_class_p): New.
(mn10300_legitimize_reload_address): New.
* config/mn10300/mn10300.h (enum reg_class): Remove
DATA_OR_ADDRESS_REGS, DATA_OR_EXTENDED_REGS, ADDRESS_OR_EXTENDED_REGS,
SP_OR_EXTENDED_REGS, SP_OR_ADDRESS_OR_EXTENDED_REGS. Add
SP_OR_GENERAL_REGS.
(REG_CLASS_NAMES): Update to match.
(REG_CLASS_CONTENTS): Likewise.
(INDEX_REG_CLASS): Use GENERAL_REGS for AM33.
(BASE_REG_CLASS): Use SP_OR_GENERAL_REGS for AM33.
(REGNO_IN_RANGE_P): Remove.
(REGNO_DATA_P): Use mn10300_regno_in_class_p.
(REGNO_ADDRESS_P, REGNO_EXTENDED_P): Likewise.
(REGNO_STRICT_OK_FOR_BASE_P): Likewise.
(REGNO_STRICT_OK_FOR_BIT_BASE_P): Likewise.
(REGNO_STRICT_OK_FOR_INDEX_P): Likewise.
(REGNO_SP_P, REGNO_AM33_P, REGNO_FP_P): Remove.
(REGNO_GENERAL_P): New.
(HAVE_POST_MODIFY_DISP): New.
(USE_LOAD_POST_INCREMENT, USE_STORE_POST_INCREMENT): New.
(LEGITIMIZE_RELOAD_ADDRESS): New.
* config/mn10300/mn10300-protos.h: Update.
* config/mn10300/mn10300.c (mn10300_preferred_reload_class): Allow * config/mn10300/mn10300.c (mn10300_preferred_reload_class): Allow
DATA_REGS for AM33 stack-pointer destination. DATA_REGS for AM33 stack-pointer destination.
(mn10300_preferred_output_reload_class): Likewise. (mn10300_preferred_output_reload_class): Likewise.
......
...@@ -26,6 +26,7 @@ ...@@ -26,6 +26,7 @@
#ifdef RTX_CODE #ifdef RTX_CODE
extern rtx mn10300_legitimize_pic_address (rtx, rtx); extern rtx mn10300_legitimize_pic_address (rtx, rtx);
extern int mn10300_legitimate_pic_operand_p (rtx); extern int mn10300_legitimate_pic_operand_p (rtx);
extern rtx mn10300_legitimize_reload_address (rtx, Mmode, int, int, int);
extern bool mn10300_function_value_regno_p (const unsigned int); extern bool mn10300_function_value_regno_p (const unsigned int);
extern int mn10300_get_live_callee_saved_regs (void); extern int mn10300_get_live_callee_saved_regs (void);
extern bool mn10300_hard_regno_mode_ok (unsigned int, Mmode); extern bool mn10300_hard_regno_mode_ok (unsigned int, Mmode);
...@@ -40,6 +41,7 @@ extern int mn10300_store_multiple_operation (rtx, Mmode); ...@@ -40,6 +41,7 @@ extern int mn10300_store_multiple_operation (rtx, Mmode);
extern int mn10300_symbolic_operand (rtx, Mmode); extern int mn10300_symbolic_operand (rtx, Mmode);
#endif /* RTX_CODE */ #endif /* RTX_CODE */
extern bool mn10300_regno_in_class_p (unsigned, int, bool);
extern int mn10300_can_use_return_insn (void); extern int mn10300_can_use_return_insn (void);
extern void mn10300_expand_prologue (void); extern void mn10300_expand_prologue (void);
extern void mn10300_expand_epilogue (void); extern void mn10300_expand_epilogue (void);
......
...@@ -495,26 +495,38 @@ mn10300_print_operand_address (FILE *file, rtx addr) ...@@ -495,26 +495,38 @@ mn10300_print_operand_address (FILE *file, rtx addr)
switch (GET_CODE (addr)) switch (GET_CODE (addr))
{ {
case POST_INC: case POST_INC:
mn10300_print_operand_address (file, XEXP (addr, 0)); mn10300_print_operand (file, XEXP (addr, 0), 0);
fputc ('+', file); fputc ('+', file);
break; break;
case POST_MODIFY:
mn10300_print_operand (file, XEXP (addr, 0), 0);
fputc ('+', file);
fputc (',', file);
mn10300_print_operand (file, XEXP (addr, 1), 0);
break;
case REG: case REG:
mn10300_print_operand (file, addr, 0); mn10300_print_operand (file, addr, 0);
break; break;
case PLUS: case PLUS:
{ {
rtx base, index; rtx base = XEXP (addr, 0);
if (REG_P (XEXP (addr, 0)) rtx index = XEXP (addr, 1);
&& REG_OK_FOR_BASE_P (XEXP (addr, 0)))
base = XEXP (addr, 0), index = XEXP (addr, 1); if (REG_P (index) && !REG_OK_FOR_INDEX_P (index))
else if (REG_P (XEXP (addr, 1)) {
&& REG_OK_FOR_BASE_P (XEXP (addr, 1))) rtx x = base;
base = XEXP (addr, 1), index = XEXP (addr, 0); base = index;
else index = x;
gcc_unreachable ();
gcc_assert (REG_P (index) && REG_OK_FOR_INDEX_P (index));
}
gcc_assert (REG_OK_FOR_BASE_P (base));
mn10300_print_operand (file, index, 0); mn10300_print_operand (file, index, 0);
fputc (',', file); fputc (',', file);
mn10300_print_operand (file, base, 0);; mn10300_print_operand (file, base, 0);
break; break;
} }
case SYMBOL_REF: case SYMBOL_REF:
...@@ -1395,8 +1407,7 @@ mn10300_secondary_reload (bool in_p, rtx x, reg_class_t rclass_i, ...@@ -1395,8 +1407,7 @@ mn10300_secondary_reload (bool in_p, rtx x, reg_class_t rclass_i,
if (in_p if (in_p
&& rclass != SP_REGS && rclass != SP_REGS
&& rclass != SP_OR_ADDRESS_REGS && rclass != SP_OR_ADDRESS_REGS
&& rclass != SP_OR_EXTENDED_REGS && rclass != SP_OR_GENERAL_REGS
&& rclass != SP_OR_ADDRESS_OR_EXTENDED_REGS
&& GET_CODE (x) == PLUS && GET_CODE (x) == PLUS
&& (XEXP (x, 0) == stack_pointer_rtx && (XEXP (x, 0) == stack_pointer_rtx
|| XEXP (x, 1) == stack_pointer_rtx)) || XEXP (x, 1) == stack_pointer_rtx))
...@@ -1422,7 +1433,7 @@ mn10300_secondary_reload (bool in_p, rtx x, reg_class_t rclass_i, ...@@ -1422,7 +1433,7 @@ mn10300_secondary_reload (bool in_p, rtx x, reg_class_t rclass_i,
addr = XEXP (x, 0); addr = XEXP (x, 0);
if (addr && CONSTANT_ADDRESS_P (addr)) if (addr && CONSTANT_ADDRESS_P (addr))
return DATA_OR_EXTENDED_REGS; return GENERAL_REGS;
} }
/* Otherwise assume no secondary reloads are needed. */ /* Otherwise assume no secondary reloads are needed. */
...@@ -1954,51 +1965,99 @@ mn10300_legitimate_pic_operand_p (rtx x) ...@@ -1954,51 +1965,99 @@ mn10300_legitimate_pic_operand_p (rtx x)
static bool static bool
mn10300_legitimate_address_p (enum machine_mode mode, rtx x, bool strict) mn10300_legitimate_address_p (enum machine_mode mode, rtx x, bool strict)
{ {
if (CONSTANT_ADDRESS_P (x) rtx base, index;
&& (! flag_pic || mn10300_legitimate_pic_operand_p (x)))
return TRUE; if (CONSTANT_ADDRESS_P (x))
return !flag_pic || mn10300_legitimate_pic_operand_p (x);
if (RTX_OK_FOR_BASE_P (x, strict)) if (RTX_OK_FOR_BASE_P (x, strict))
return TRUE; return true;
if (TARGET_AM33 if (TARGET_AM33 && (mode == SImode || mode == SFmode || mode == HImode))
&& GET_CODE (x) == POST_INC {
&& RTX_OK_FOR_BASE_P (XEXP (x, 0), strict) if (GET_CODE (x) == POST_INC)
&& (mode == SImode || mode == SFmode || mode == HImode)) return RTX_OK_FOR_BASE_P (XEXP (x, 0), strict);
return TRUE; if (GET_CODE (x) == POST_MODIFY)
return (RTX_OK_FOR_BASE_P (XEXP (x, 0), strict)
&& CONSTANT_ADDRESS_P (XEXP (x, 1)));
}
if (GET_CODE (x) == PLUS) if (GET_CODE (x) != PLUS)
return false;
base = XEXP (x, 0);
index = XEXP (x, 1);
if (!REG_P (base))
return false;
if (REG_P (index))
{ {
rtx base = 0, index = 0; /* ??? Without AM33 generalized (Ri,Rn) addressing, reg+reg
addressing is hard to satisfy. */
if (!TARGET_AM33)
return false;
if (REG_P (XEXP (x, 0)) return (REGNO_GENERAL_P (REGNO (base), strict)
&& REGNO_STRICT_OK_FOR_BASE_P (REGNO (XEXP (x, 0)), strict)) && REGNO_GENERAL_P (REGNO (index), strict));
{ }
base = XEXP (x, 0);
index = XEXP (x, 1);
}
if (REG_P (XEXP (x, 1)) if (!REGNO_STRICT_OK_FOR_BASE_P (REGNO (base), strict))
&& REGNO_STRICT_OK_FOR_BASE_P (REGNO (XEXP (x, 1)), strict)) return false;
{
base = XEXP (x, 1);
index = XEXP (x, 0);
}
if (base != 0 && index != 0) if (CONST_INT_P (index))
{ return IN_RANGE (INTVAL (index), -1 - 0x7fffffff, 0x7fffffff);
if (CONST_INT_P (index))
return TRUE; if (CONSTANT_ADDRESS_P (index))
if (GET_CODE (index) == CONST return !flag_pic || mn10300_legitimate_pic_operand_p (index);
&& GET_CODE (XEXP (index, 0)) != PLUS
&& (! flag_pic return false;
|| (mn10300_legitimate_pic_operand_p (index) }
&& GET_MODE_SIZE (mode) == 4)))
return TRUE; bool
} mn10300_regno_in_class_p (unsigned regno, int rclass, bool strict)
{
if (regno >= FIRST_PSEUDO_REGISTER)
{
if (!strict)
return true;
if (!reg_renumber)
return false;
regno = reg_renumber[regno];
}
return TEST_HARD_REG_BIT (reg_class_contents[rclass], regno);
}
rtx
mn10300_legitimize_reload_address (rtx x,
enum machine_mode mode ATTRIBUTE_UNUSED,
int opnum, int type,
int ind_levels ATTRIBUTE_UNUSED)
{
bool any_change = false;
/* See above re disabling reg+reg addressing for MN103. */
if (!TARGET_AM33)
return NULL_RTX;
if (GET_CODE (x) != PLUS)
return NULL_RTX;
if (XEXP (x, 0) == stack_pointer_rtx)
{
push_reload (XEXP (x, 0), NULL_RTX, &XEXP (x, 0), NULL,
GENERAL_REGS, GET_MODE (x), VOIDmode, 0, 0,
opnum, (enum reload_type) type);
any_change = true;
}
if (XEXP (x, 1) == stack_pointer_rtx)
{
push_reload (XEXP (x, 1), NULL_RTX, &XEXP (x, 1), NULL,
GENERAL_REGS, GET_MODE (x), VOIDmode, 0, 0,
opnum, (enum reload_type) type);
any_change = true;
} }
return FALSE; return any_change ? x : NULL_RTX;
} }
/* Used by LEGITIMATE_CONSTANT_P(). Returns TRUE if X is a valid /* Used by LEGITIMATE_CONSTANT_P(). Returns TRUE if X is a valid
......
...@@ -261,12 +261,9 @@ extern enum processor_type mn10300_tune_cpu; ...@@ -261,12 +261,9 @@ extern enum processor_type mn10300_tune_cpu;
enum reg_class enum reg_class
{ {
NO_REGS, DATA_REGS, ADDRESS_REGS, SP_REGS, NO_REGS, DATA_REGS, ADDRESS_REGS, SP_REGS, SP_OR_ADDRESS_REGS,
DATA_OR_ADDRESS_REGS, SP_OR_ADDRESS_REGS, EXTENDED_REGS, FP_REGS, FP_ACC_REGS, CC_REGS,
EXTENDED_REGS, DATA_OR_EXTENDED_REGS, ADDRESS_OR_EXTENDED_REGS, GENERAL_REGS, SP_OR_GENERAL_REGS, ALL_REGS, LIM_REG_CLASSES
SP_OR_EXTENDED_REGS, SP_OR_ADDRESS_OR_EXTENDED_REGS,
FP_REGS, FP_ACC_REGS, CC_REGS,
GENERAL_REGS, ALL_REGS, LIM_REG_CLASSES
}; };
#define N_REG_CLASSES (int) LIM_REG_CLASSES #define N_REG_CLASSES (int) LIM_REG_CLASSES
...@@ -274,13 +271,9 @@ enum reg_class ...@@ -274,13 +271,9 @@ enum reg_class
/* Give names of register classes as strings for dump file. */ /* Give names of register classes as strings for dump file. */
#define REG_CLASS_NAMES \ #define REG_CLASS_NAMES \
{ "NO_REGS", "DATA_REGS", "ADDRESS_REGS", \ { "NO_REGS", "DATA_REGS", "ADDRESS_REGS", "SP_REGS", "SP_OR_ADDRESS_REGS", \
"SP_REGS", "DATA_OR_ADDRESS_REGS", "SP_OR_ADDRESS_REGS", \ "EXTENDED_REGS", "FP_REGS", "FP_ACC_REGS", "CC_REGS", \
"EXTENDED_REGS", \ "GENERAL_REGS", "SP_OR_GENERAL_REGS", "ALL_REGS", "LIM_REGS" \
"DATA_OR_EXTENDED_REGS", "ADDRESS_OR_EXTENDED_REGS", \
"SP_OR_EXTENDED_REGS", "SP_OR_ADDRESS_OR_EXTENDED_REGS", \
"FP_REGS", "FP_ACC_REGS", "CC_REGS", \
"GENERAL_REGS", "ALL_REGS", "LIM_REGS" \
} }
/* Define which registers fit in which classes. /* Define which registers fit in which classes.
...@@ -292,17 +285,13 @@ enum reg_class ...@@ -292,17 +285,13 @@ enum reg_class
{ 0x0000000f, 0 }, /* DATA_REGS */ \ { 0x0000000f, 0 }, /* DATA_REGS */ \
{ 0x000001f0, 0 }, /* ADDRESS_REGS */ \ { 0x000001f0, 0 }, /* ADDRESS_REGS */ \
{ 0x00000200, 0 }, /* SP_REGS */ \ { 0x00000200, 0 }, /* SP_REGS */ \
{ 0x000001ff, 0 }, /* DATA_OR_ADDRESS_REGS */ \
{ 0x000003f0, 0 }, /* SP_OR_ADDRESS_REGS */ \ { 0x000003f0, 0 }, /* SP_OR_ADDRESS_REGS */ \
{ 0x0003fc00, 0 }, /* EXTENDED_REGS */ \ { 0x0003fc00, 0 }, /* EXTENDED_REGS */ \
{ 0x0003fc0f, 0 }, /* DATA_OR_EXTENDED_REGS */ \
{ 0x0003fdf0, 0 }, /* ADDRESS_OR_EXTENDED_REGS */ \
{ 0x0003fe00, 0 }, /* SP_OR_EXTENDED_REGS */ \
{ 0x0003fff0, 0 }, /* SP_OR_ADDRESS_OR_EXTENDED_REGS */ \
{ 0xfffc0000, 0x3ffff },/* FP_REGS */ \ { 0xfffc0000, 0x3ffff },/* FP_REGS */ \
{ 0x03fc0000, 0 }, /* FP_ACC_REGS */ \ { 0x03fc0000, 0 }, /* FP_ACC_REGS */ \
{ 0x00000000, 0x80000 },/* CC_REGS */ \ { 0x00000000, 0x80000 },/* CC_REGS */ \
{ 0x0003fdff, 0 }, /* GENERAL_REGS */ \ { 0x0003fdff, 0 }, /* GENERAL_REGS */ \
{ 0x0003ffff, 0 }, /* SP_OR_GENERAL_REGS */ \
{ 0xffffffff, 0xfffff } /* ALL_REGS */ \ { 0xffffffff, 0xfffff } /* ALL_REGS */ \
} }
...@@ -334,8 +323,10 @@ enum reg_class ...@@ -334,8 +323,10 @@ enum reg_class
NO_REGS) NO_REGS)
/* The class value for index registers, and the one for base regs. */ /* The class value for index registers, and the one for base regs. */
#define INDEX_REG_CLASS DATA_OR_EXTENDED_REGS #define INDEX_REG_CLASS \
#define BASE_REG_CLASS SP_OR_ADDRESS_REGS (TARGET_AM33 ? GENERAL_REGS : DATA_REGS)
#define BASE_REG_CLASS \
(TARGET_AM33 ? SP_OR_GENERAL_REGS : SP_OR_ADDRESS_REGS)
/* Macros to check register numbers against specific register classes. */ /* Macros to check register numbers against specific register classes. */
...@@ -364,50 +355,31 @@ enum reg_class ...@@ -364,50 +355,31 @@ enum reg_class
# define REG_STRICT 1 # define REG_STRICT 1
#endif #endif
# define REGNO_IN_RANGE_P(regno,min,max,strict) \
(IN_RANGE ((regno), (min), (max)) \
|| ((strict) \
? (reg_renumber \
&& reg_renumber[(regno)] >= (min) \
&& reg_renumber[(regno)] <= (max)) \
: (regno) >= FIRST_PSEUDO_REGISTER))
#define REGNO_DATA_P(regno, strict) \ #define REGNO_DATA_P(regno, strict) \
(REGNO_IN_RANGE_P ((regno), FIRST_DATA_REGNUM, LAST_DATA_REGNUM, \ mn10300_regno_in_class_p (regno, DATA_REGS, strict)
(strict)))
#define REGNO_ADDRESS_P(regno, strict) \ #define REGNO_ADDRESS_P(regno, strict) \
(REGNO_IN_RANGE_P ((regno), FIRST_ADDRESS_REGNUM, LAST_ADDRESS_REGNUM, \ mn10300_regno_in_class_p (regno, ADDRESS_REGS, strict)
(strict)))
#define REGNO_SP_P(regno, strict) \
(REGNO_IN_RANGE_P ((regno), STACK_POINTER_REGNUM, STACK_POINTER_REGNUM, \
(strict)))
#define REGNO_EXTENDED_P(regno, strict) \ #define REGNO_EXTENDED_P(regno, strict) \
(REGNO_IN_RANGE_P ((regno), FIRST_EXTENDED_REGNUM, LAST_EXTENDED_REGNUM, \ mn10300_regno_in_class_p (regno, EXTENDED_REGS, strict)
(strict))) #define REGNO_GENERAL_P(regno, strict) \
#define REGNO_AM33_P(regno, strict) \ mn10300_regno_in_class_p (regno, GENERAL_REGS, strict)
(REGNO_DATA_P ((regno), (strict)) || REGNO_ADDRESS_P ((regno), (strict)) \
|| REGNO_EXTENDED_P ((regno), (strict)))
#define REGNO_FP_P(regno, strict) \
(REGNO_IN_RANGE_P ((regno), FIRST_FP_REGNUM, LAST_FP_REGNUM, (strict)))
#define REGNO_STRICT_OK_FOR_BASE_P(regno, strict) \ #define REGNO_STRICT_OK_FOR_BASE_P(regno, strict) \
(REGNO_SP_P ((regno), (strict)) \ mn10300_regno_in_class_p (regno, BASE_REG_CLASS, strict)
|| REGNO_ADDRESS_P ((regno), (strict)) \
|| REGNO_EXTENDED_P ((regno), (strict)))
#define REGNO_OK_FOR_BASE_P(regno) \ #define REGNO_OK_FOR_BASE_P(regno) \
(REGNO_STRICT_OK_FOR_BASE_P ((regno), REG_STRICT)) (REGNO_STRICT_OK_FOR_BASE_P ((regno), REG_STRICT))
#define REG_OK_FOR_BASE_P(X) \ #define REG_OK_FOR_BASE_P(X) \
(REGNO_OK_FOR_BASE_P (REGNO (X))) (REGNO_OK_FOR_BASE_P (REGNO (X)))
#define REGNO_STRICT_OK_FOR_BIT_BASE_P(regno, strict) \ #define REGNO_STRICT_OK_FOR_BIT_BASE_P(regno, strict) \
(REGNO_SP_P ((regno), (strict)) || REGNO_ADDRESS_P ((regno), (strict))) mn10300_regno_in_class_p (regno, ADDRESS_REGS, strict)
#define REGNO_OK_FOR_BIT_BASE_P(regno) \ #define REGNO_OK_FOR_BIT_BASE_P(regno) \
(REGNO_STRICT_OK_FOR_BIT_BASE_P ((regno), REG_STRICT)) (REGNO_STRICT_OK_FOR_BIT_BASE_P ((regno), REG_STRICT))
#define REG_OK_FOR_BIT_BASE_P(X) \ #define REG_OK_FOR_BIT_BASE_P(X) \
(REGNO_OK_FOR_BIT_BASE_P (REGNO (X))) (REGNO_OK_FOR_BIT_BASE_P (REGNO (X)))
#define REGNO_STRICT_OK_FOR_INDEX_P(regno, strict) \ #define REGNO_STRICT_OK_FOR_INDEX_P(regno, strict) \
(REGNO_DATA_P ((regno), (strict)) || REGNO_EXTENDED_P ((regno), (strict))) mn10300_regno_in_class_p (regno, INDEX_REG_CLASS, strict)
#define REGNO_OK_FOR_INDEX_P(regno) \ #define REGNO_OK_FOR_INDEX_P(regno) \
(REGNO_STRICT_OK_FOR_INDEX_P ((regno), REG_STRICT)) (REGNO_STRICT_OK_FOR_INDEX_P ((regno), REG_STRICT))
#define REG_OK_FOR_INDEX_P(X) \ #define REG_OK_FOR_INDEX_P(X) \
...@@ -557,7 +529,15 @@ struct cum_arg ...@@ -557,7 +529,15 @@ struct cum_arg
#define MAX_REGS_PER_ADDRESS 2 #define MAX_REGS_PER_ADDRESS 2
#define HAVE_POST_INCREMENT (TARGET_AM33) /* We have post-increments. */
#define HAVE_POST_INCREMENT TARGET_AM33
#define HAVE_POST_MODIFY_DISP TARGET_AM33
/* ... But we don't want to use them for block moves. Small offsets are
just as effective, at least for inline block move sizes, and appears
to produce cleaner code. */
#define USE_LOAD_POST_INCREMENT(M) 0
#define USE_STORE_POST_INCREMENT(M) 0
/* Accept either REG or SUBREG where a register is valid. */ /* Accept either REG or SUBREG where a register is valid. */
...@@ -568,6 +548,15 @@ struct cum_arg ...@@ -568,6 +548,15 @@ struct cum_arg
&& REGNO_STRICT_OK_FOR_BASE_P (REGNO (SUBREG_REG (X)), \ && REGNO_STRICT_OK_FOR_BASE_P (REGNO (SUBREG_REG (X)), \
(strict)))) (strict))))
#define LEGITIMIZE_RELOAD_ADDRESS(X,MODE,OPNUM,TYPE,IND_L,WIN) \
do { \
rtx new_x = mn10300_legitimize_reload_address (X, MODE, OPNUM, TYPE, IND_L); \
if (new_x) \
{ \
X = new_x; \
goto WIN; \
} \
} while (0)
/* Nonzero if the constant value X is a legitimate general operand. /* Nonzero if the constant value X is a legitimate general operand.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment