Commit 2999cea5 authored by Mythri A's avatar Mythri A Committed by Commit Bot

Reland "[ic] Don't transition to premonomorphic state"

This is a reland of 159df248

Original change's description:
> [ic] Don't transition to premonomorphic state
> 
> We used to use premonomorphic state to delay initializing the ICs.
> This optimization was to avoid the cost of setting up handlers if the
> code executed only once. With lazy feedback allocation we no longer
> need this.
> 
> This cl also renames LoadIC_Uninitialized to LoadIC_Nofeedback and
> StoreIC_Uninitialized to StoreIC_Nofeedback since we now miss to
> runtime in the uninitialized state and use the builtin when there
> is no feedback.
> 
> 
> Change-Id: I1633e61ea74664da51348e362c34c47a017a264a
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1683525
> Commit-Queue: Mythri Alle <mythria@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#63020}

Change-Id: Ica7eb65649615c2f8410d5b815a98b55cb1cfc4d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1731000
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: 's avatarToon Verwaest <verwaest@chromium.org>
Reviewed-by: 's avatarRoss McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63082}
parent 8fdb2387
......@@ -222,9 +222,9 @@ namespace internal {
TFH(LoadIC_Slow, LoadWithVector) \
TFH(LoadIC_StringLength, LoadWithVector) \
TFH(LoadIC_StringWrapperLength, LoadWithVector) \
TFH(LoadIC_Uninitialized, LoadWithVector) \
TFH(LoadIC_NoFeedback, Load) \
TFH(StoreGlobalIC_Slow, StoreWithVector) \
TFH(StoreIC_Uninitialized, StoreWithVector) \
TFH(StoreIC_NoFeedback, Store) \
TFH(StoreInArrayLiteralIC_Slow, StoreWithVector) \
TFH(KeyedLoadIC_SloppyArguments, LoadWithVector) \
TFH(LoadIndexedInterceptorIC, LoadWithVector) \
......
......@@ -66,9 +66,9 @@ void Builtins::Generate_KeyedStoreIC_Megamorphic(
KeyedStoreGenericGenerator::Generate(state);
}
void Builtins::Generate_StoreIC_Uninitialized(
void Builtins::Generate_StoreIC_NoFeedback(
compiler::CodeAssemblerState* state) {
StoreICUninitializedGenerator::Generate(state);
StoreICNoFeedbackGenerator::Generate(state);
}
// TODO(mythria): Check if we can remove feedback vector and slot parameters in
......
......@@ -23,7 +23,7 @@ namespace internal {
IC_BUILTIN(LoadIC)
IC_BUILTIN(LoadIC_Megamorphic)
IC_BUILTIN(LoadIC_Noninlined)
IC_BUILTIN(LoadIC_Uninitialized)
IC_BUILTIN(LoadIC_NoFeedback)
IC_BUILTIN(LoadICTrampoline)
IC_BUILTIN(LoadICTrampoline_Megamorphic)
IC_BUILTIN(KeyedLoadIC)
......
......@@ -2298,8 +2298,7 @@ bool Compiler::FinalizeOptimizedCompilationJob(OptimizedCompilationJob* job,
return CompilationJob::FAILED;
}
void Compiler::PostInstantiation(Handle<JSFunction> function,
AllocationType allocation) {
void Compiler::PostInstantiation(Handle<JSFunction> function) {
Isolate* isolate = function->GetIsolate();
Handle<SharedFunctionInfo> shared(function->shared(), isolate);
IsCompiledScope is_compiled_scope(shared->is_compiled_scope());
......
......@@ -86,7 +86,7 @@ class V8_EXPORT_PRIVATE Compiler : public AllStatic {
// Give the compiler a chance to perform low-latency initialization tasks of
// the given {function} on its instantiation. Note that only the runtime will
// offer this chance, optimized closure instantiation will not call this.
static void PostInstantiation(Handle<JSFunction> function, AllocationType);
static void PostInstantiation(Handle<JSFunction> function);
// Parser::Parse, then Compiler::Analyze.
static bool ParseAndAnalyze(ParseInfo* parse_info,
......
......@@ -2510,7 +2510,7 @@ Handle<JSFunction> Factory::NewFunctionFromSharedFunctionInfo(
NewFunction(initial_map, info, context, allocation);
// Give compiler a chance to pre-initialize.
Compiler::PostInstantiation(result, allocation);
Compiler::PostInstantiation(result);
return result;
}
......@@ -2542,7 +2542,7 @@ Handle<JSFunction> Factory::NewFunctionFromSharedFunctionInfo(
result->set_raw_feedback_cell(*feedback_cell);
// Give compiler a chance to pre-initialize.
Compiler::PostInstantiation(result, allocation);
Compiler::PostInstantiation(result);
return result;
}
......
......@@ -2524,8 +2524,8 @@ void AccessorAssembler::LoadIC_BytecodeHandler(const LazyLoadICParameters* p,
Comment("LoadIC_BytecodeHandler_nofeedback");
// Call into the stub that implements the non-inlined parts of LoadIC.
exit_point->ReturnCallStub(
Builtins::CallableFor(isolate(), Builtins::kLoadIC_Uninitialized),
p->context(), p->receiver(), p->name(), p->slot(), p->vector());
Builtins::CallableFor(isolate(), Builtins::kLoadIC_NoFeedback),
p->context(), p->receiver(), p->name(), p->slot());
}
BIND(&miss);
......@@ -2589,8 +2589,6 @@ void AccessorAssembler::LoadIC_Noninlined(const LoadICParameters* p,
TVariable<MaybeObject>* var_handler,
Label* if_handler, Label* miss,
ExitPoint* exit_point) {
Label try_uninitialized(this, Label::kDeferred);
// Neither deprecated map nor monomorphic. These cases are handled in the
// bytecode handler.
CSA_ASSERT(this, Word32BinaryNot(IsDeprecatedMap(receiver_map)));
......@@ -2601,41 +2599,20 @@ void AccessorAssembler::LoadIC_Noninlined(const LoadICParameters* p,
{
// Check megamorphic case.
GotoIfNot(WordEqual(feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
&try_uninitialized);
miss);
TryProbeStubCache(isolate()->load_stub_cache(), p->receiver(), p->name(),
if_handler, var_handler, miss);
}
BIND(&try_uninitialized);
{
// Check uninitialized case.
GotoIfNot(WordEqual(feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
miss);
exit_point->ReturnCallStub(
Builtins::CallableFor(isolate(), Builtins::kLoadIC_Uninitialized),
p->context(), p->receiver(), p->name(), p->slot(), p->vector());
}
}
void AccessorAssembler::LoadIC_Uninitialized(const LoadICParameters* p) {
Label miss(this, Label::kDeferred),
check_function_prototype(this);
void AccessorAssembler::LoadIC_NoFeedback(const LoadICParameters* p) {
Label miss(this, Label::kDeferred);
Node* receiver = p->receiver();
GotoIf(TaggedIsSmi(receiver), &miss);
Node* receiver_map = LoadMap(receiver);
Node* instance_type = LoadMapInstanceType(receiver_map);
GotoIf(IsUndefined(p->vector()), &check_function_prototype);
// Optimistically write the state transition to the vector.
StoreFeedbackVectorSlot(p->vector(), p->slot(),
LoadRoot(RootIndex::kpremonomorphic_symbol),
SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
StoreWeakReferenceInFeedbackVector(p->vector(), p->slot(), receiver_map,
kTaggedSize, SMI_PARAMETERS);
Goto(&check_function_prototype);
BIND(&check_function_prototype);
{
// Special case for Function.prototype load, because it's very common
// for ICs that are only executed once (MyFunc.prototype.foo = ...).
......@@ -2655,15 +2632,6 @@ void AccessorAssembler::LoadIC_Uninitialized(const LoadICParameters* p) {
BIND(&miss);
{
Label call_runtime(this, Label::kDeferred);
GotoIf(IsUndefined(p->vector()), &call_runtime);
// Undo the optimistic state transition.
StoreFeedbackVectorSlot(p->vector(), p->slot(),
LoadRoot(RootIndex::kuninitialized_symbol),
SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
Goto(&call_runtime);
BIND(&call_runtime);
TailCallRuntime(Runtime::kLoadIC_Miss, p->context(), p->receiver(),
p->name(), p->slot(), p->vector());
}
......@@ -2772,6 +2740,7 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
TVARIABLE(MaybeObject, var_handler);
Label if_handler(this, &var_handler), try_polymorphic(this, Label::kDeferred),
try_megamorphic(this, Label::kDeferred),
try_uninitialized(this, Label::kDeferred),
try_polymorphic_name(this, Label::kDeferred),
miss(this, Label::kDeferred), generic(this, Label::kDeferred);
......@@ -2808,7 +2777,7 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
// Check megamorphic case.
Comment("KeyedLoadIC_try_megamorphic");
Branch(WordEqual(strong_feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
&generic, &try_polymorphic_name);
&generic, &try_uninitialized);
}
BIND(&generic);
......@@ -2821,6 +2790,15 @@ void AccessorAssembler::KeyedLoadIC(const LoadICParameters* p,
p->vector());
}
BIND(&try_uninitialized);
{
// Check uninitialized case.
Comment("KeyedLoadIC_try_uninitialized");
Branch(
WordEqual(strong_feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
&miss, &try_polymorphic_name);
}
BIND(&try_polymorphic_name);
{
// We might have a name in feedback, and a weak fixed array in the next
......@@ -3014,8 +2992,7 @@ void AccessorAssembler::StoreIC(const StoreICParameters* p) {
Label if_handler(this, &var_handler),
if_handler_from_stub_cache(this, &var_handler, Label::kDeferred),
try_polymorphic(this, Label::kDeferred),
try_megamorphic(this, Label::kDeferred),
try_uninitialized(this, Label::kDeferred), miss(this, Label::kDeferred),
try_megamorphic(this, Label::kDeferred), miss(this, Label::kDeferred),
no_feedback(this, Label::kDeferred);
Node* receiver_map = LoadReceiverMap(p->receiver());
......@@ -3049,24 +3026,16 @@ void AccessorAssembler::StoreIC(const StoreICParameters* p) {
// Check megamorphic case.
GotoIfNot(
WordEqual(strong_feedback, LoadRoot(RootIndex::kmegamorphic_symbol)),
&try_uninitialized);
&miss);
TryProbeStubCache(isolate()->store_stub_cache(), p->receiver(), p->name(),
&if_handler, &var_handler, &miss);
}
BIND(&try_uninitialized);
{
// Check uninitialized case.
Branch(
WordEqual(strong_feedback, LoadRoot(RootIndex::kuninitialized_symbol)),
&no_feedback, &miss);
}
BIND(&no_feedback);
{
TailCallBuiltin(Builtins::kStoreIC_Uninitialized, p->context(),
p->receiver(), p->name(), p->value(), p->slot(),
p->vector());
TailCallBuiltin(Builtins::kStoreIC_NoFeedback, p->context(), p->receiver(),
p->name(), p->value(), p->slot());
}
BIND(&miss);
......@@ -3085,6 +3054,10 @@ void AccessorAssembler::StoreGlobalIC(const StoreICParameters* pp) {
BIND(&if_heapobject);
{
Label try_handler(this), miss(this, Label::kDeferred);
// We use pre-monomorphic state for global stores that run into
// interceptors because the property doesn't exist yet. Using
// pre-monomorphic state gives it a chance to find more information the
// second time.
GotoIf(
WordEqual(maybe_weak_ref, LoadRoot(RootIndex::kpremonomorphic_symbol)),
&miss);
......@@ -3432,17 +3405,16 @@ void AccessorAssembler::GenerateLoadIC_Noninlined() {
slot, vector);
}
void AccessorAssembler::GenerateLoadIC_Uninitialized() {
using Descriptor = LoadWithVectorDescriptor;
void AccessorAssembler::GenerateLoadIC_NoFeedback() {
using Descriptor = LoadDescriptor;
Node* receiver = Parameter(Descriptor::kReceiver);
Node* name = Parameter(Descriptor::kName);
Node* slot = Parameter(Descriptor::kSlot);
Node* vector = Parameter(Descriptor::kVector);
TNode<Context> context = CAST(Parameter(Descriptor::kContext));
LoadICParameters p(context, receiver, name, slot, vector);
LoadIC_Uninitialized(&p);
LoadICParameters p(context, receiver, name, slot, UndefinedConstant());
LoadIC_NoFeedback(&p);
}
void AccessorAssembler::GenerateLoadICTrampoline() {
......
......@@ -30,7 +30,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
void GenerateLoadIC();
void GenerateLoadIC_Megamorphic();
void GenerateLoadIC_Noninlined();
void GenerateLoadIC_Uninitialized();
void GenerateLoadIC_NoFeedback();
void GenerateLoadICTrampoline();
void GenerateLoadICTrampoline_Megamorphic();
void GenerateKeyedLoadIC();
......@@ -214,7 +214,7 @@ class V8_EXPORT_PRIVATE AccessorAssembler : public CodeStubAssembler {
TNode<MaybeObject> LoadDescriptorValueOrFieldType(
TNode<Map> map, TNode<IntPtrT> descriptor_entry);
void LoadIC_Uninitialized(const LoadICParameters* p);
void LoadIC_NoFeedback(const LoadICParameters* p);
void KeyedLoadIC(const LoadICParameters* p, LoadAccessMode access_mode);
void KeyedLoadICGeneric(const LoadICParameters* p);
......
......@@ -400,10 +400,8 @@ MaybeHandle<Object> LoadIC::Load(Handle<Object> object, Handle<Name> name) {
if (MigrateDeprecated(isolate(), object)) use_ic = false;
if (state() != UNINITIALIZED) {
JSObject::MakePrototypesFast(object, kStartAtReceiver, isolate());
update_receiver_map(object);
}
JSObject::MakePrototypesFast(object, kStartAtReceiver, isolate());
update_receiver_map(object);
LookupIterator it(isolate(), object, name);
......@@ -648,15 +646,6 @@ void IC::PatchCache(Handle<Name> name, const MaybeObjectHandle& handler) {
}
void LoadIC::UpdateCaches(LookupIterator* lookup) {
if (state() == UNINITIALIZED && !IsLoadGlobalIC()) {
// This is the first time we execute this inline cache. Set the target to
// the pre monomorphic stub to delay setting the monomorphic state.
TRACE_HANDLER_STATS(isolate(), LoadIC_Premonomorphic);
ConfigureVectorState(receiver_map());
TraceIC("LoadIC", lookup->name());
return;
}
Handle<Object> code;
if (lookup->state() == LookupIterator::ACCESS_CHECK) {
code = slow_stub();
......@@ -1415,9 +1404,7 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
return TypeError(MessageTemplate::kNonObjectPropertyStore, object, name);
}
if (state() != UNINITIALIZED) {
JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());
}
JSObject::MakePrototypesFast(object, kStartAtPrototype, isolate());
LookupIterator it(isolate(), object, name);
if (name->IsPrivate()) {
......@@ -1442,15 +1429,6 @@ MaybeHandle<Object> StoreIC::Store(Handle<Object> object, Handle<Name> name,
void StoreIC::UpdateCaches(LookupIterator* lookup, Handle<Object> value,
StoreOrigin store_origin) {
if (state() == UNINITIALIZED && !IsStoreGlobalIC()) {
// This is the first time we execute this inline cache. Transition
// to premonomorphic state to delay setting the monomorphic state.
TRACE_HANDLER_STATS(isolate(), StoreIC_Premonomorphic);
ConfigureVectorState(receiver_map());
TraceIC("StoreIC", lookup->name());
return;
}
MaybeObjectHandle handler;
if (LookupForWrite(lookup, value, store_origin)) {
if (IsStoreGlobalIC()) {
......@@ -1810,10 +1788,8 @@ void KeyedStoreIC::UpdateStoreElement(Handle<Map> receiver_map,
handlers.reserve(target_receiver_maps.size());
StoreElementPolymorphicHandlers(&target_receiver_maps, &handlers, store_mode);
if (target_receiver_maps.size() == 0) {
// Transition to PREMONOMORPHIC state here and remember a weak-reference
// to the {receiver_map} in case TurboFan sees this function before the
// IC can transition further.
ConfigureVectorState(receiver_map);
Handle<Object> handler = StoreElementHandler(receiver_map, store_mode);
ConfigureVectorState(Handle<Name>(), receiver_map, handler);
} else if (target_receiver_maps.size() == 1) {
ConfigureVectorState(Handle<Name>(), target_receiver_maps[0], handlers[0]);
} else {
......
......@@ -30,7 +30,7 @@ class KeyedStoreGenericAssembler : public AccessorAssembler {
void KeyedStoreGeneric();
void StoreIC_Uninitialized();
void StoreIC_NoFeedback();
// Generates code for [[Set]] operation, the |unique_name| is supposed to be
// unique otherwise this code will always go to runtime.
......@@ -138,10 +138,9 @@ void KeyedStoreGenericGenerator::Generate(compiler::CodeAssemblerState* state) {
assembler.KeyedStoreGeneric();
}
void StoreICUninitializedGenerator::Generate(
compiler::CodeAssemblerState* state) {
void StoreICNoFeedbackGenerator::Generate(compiler::CodeAssemblerState* state) {
KeyedStoreGenericAssembler assembler(state, StoreMode::kOrdinary);
assembler.StoreIC_Uninitialized();
assembler.StoreIC_NoFeedback();
}
void KeyedStoreGenericGenerator::SetProperty(
......@@ -1043,14 +1042,13 @@ void KeyedStoreGenericAssembler::SetProperty(TNode<Context> context,
KeyedStoreGeneric(context, receiver, key, value, Just(language_mode));
}
void KeyedStoreGenericAssembler::StoreIC_Uninitialized() {
using Descriptor = StoreWithVectorDescriptor;
void KeyedStoreGenericAssembler::StoreIC_NoFeedback() {
using Descriptor = StoreDescriptor;
Node* receiver = Parameter(Descriptor::kReceiver);
Node* name = Parameter(Descriptor::kName);
Node* value = Parameter(Descriptor::kValue);
Node* slot = Parameter(Descriptor::kSlot);
Node* vector = Parameter(Descriptor::kVector);
Node* context = Parameter(Descriptor::kContext);
Label miss(this, Label::kDeferred), store_property(this);
......@@ -1061,33 +1059,16 @@ void KeyedStoreGenericAssembler::StoreIC_Uninitialized() {
// Receivers requiring non-standard element accesses (interceptors, access
// checks, strings and string wrappers, proxies) are handled in the runtime.
GotoIf(IsSpecialReceiverInstanceType(instance_type), &miss);
// Optimistically write the state transition to the vector.
GotoIf(IsUndefined(vector), &store_property);
StoreFeedbackVectorSlot(vector, slot,
LoadRoot(RootIndex::kpremonomorphic_symbol),
SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
Goto(&store_property);
BIND(&store_property);
{
StoreICParameters p(CAST(context), receiver, name, value, slot, vector);
StoreICParameters p(CAST(context), receiver, name, value, slot,
UndefinedConstant());
EmitGenericPropertyStore(receiver, receiver_map, &p, &miss);
}
BIND(&miss);
{
Label call_runtime(this);
// Undo the optimistic state transition.
GotoIf(IsUndefined(vector), &call_runtime);
StoreFeedbackVectorSlot(vector, slot,
LoadRoot(RootIndex::kuninitialized_symbol),
SKIP_WRITE_BARRIER, 0, SMI_PARAMETERS);
Goto(&call_runtime);
BIND(&call_runtime);
TailCallRuntime(Runtime::kStoreIC_Miss, context, value, slot, vector,
receiver, name);
TailCallRuntime(Runtime::kStoreIC_Miss, context, value, slot,
UndefinedConstant(), receiver, name);
}
}
......
......@@ -37,7 +37,7 @@ class KeyedStoreGenericGenerator {
TNode<Object> value);
};
class StoreICUninitializedGenerator {
class StoreICNoFeedbackGenerator {
public:
static void Generate(compiler::CodeAssemblerState* state);
};
......
......@@ -310,13 +310,19 @@ namespace {
bool EnsureFeedbackVector(Handle<JSFunction> function) {
// Check function allows lazy compilation.
if (!function->shared().allows_lazy_compilation()) {
return false;
}
if (!function->shared().allows_lazy_compilation()) return false;
if (function->has_feedback_vector()) return true;
// If function isn't compiled, compile it now.
IsCompiledScope is_compiled_scope(function->shared().is_compiled_scope());
if (!is_compiled_scope.is_compiled() &&
// If the JSFunction isn't compiled but it has a initialized feedback cell
// then no need to compile. CompileLazy builtin would handle these cases by
// installing the code from SFI. Calling compile here may cause another
// optimization if FLAG_always_opt is set.
bool needs_compilation =
!function->is_compiled() && !function->has_closure_feedback_cell_array();
if (needs_compilation &&
!Compiler::Compile(function, Compiler::CLEAR_EXCEPTION,
&is_compiled_scope)) {
return false;
......
......@@ -438,9 +438,7 @@ TEST(VectorLoadICStates) {
Handle<FeedbackVector>(f->feedback_vector(), isolate);
FeedbackSlot slot(0);
FeedbackNexus nexus(feedback_vector, slot);
CHECK_EQ(PREMONOMORPHIC, nexus.ic_state());
CompileRun("f(o)");
CHECK_EQ(MONOMORPHIC, nexus.ic_state());
// Verify that the monomorphic map is the one we expect.
v8::MaybeLocal<v8::Value> v8_o =
......@@ -526,16 +524,13 @@ TEST(VectorLoadICOnSmi) {
CompileRun(
"var o = { foo: 3 };"
"%EnsureFeedbackVectorForFunction(f);"
"function f(a) { return a.foo; } f(o);");
"function f(a) { return a.foo; } f(34);");
Handle<JSFunction> f = GetFunction("f");
// There should be one IC.
Handle<FeedbackVector> feedback_vector =
Handle<FeedbackVector>(f->feedback_vector(), isolate);
FeedbackSlot slot(0);
FeedbackNexus nexus(feedback_vector, slot);
CHECK_EQ(PREMONOMORPHIC, nexus.ic_state());
CompileRun("f(34)");
CHECK_EQ(MONOMORPHIC, nexus.ic_state());
// Verify that the monomorphic map is the one we expect.
Map number_map = ReadOnlyRoots(heap).heap_number_map();
......
......@@ -71,7 +71,6 @@ TEST(DisasmPoisonPolymorphicLoad) {
"let o2 = { y : 1 };"
"o2.x = 2;"
"%PrepareFunctionForOptimization(poly);"
"poly(o1);"
"poly(o2);"
"poly(o1);"
"poly(o2);"
......
......@@ -84,7 +84,6 @@ TEST(DisasmPoisonPolymorphicLoad) {
"let o2 = { y : 1 };"
"o2.x = 2;"
"%PrepareFunctionForOptimization(poly);"
"poly(o1);"
"poly(o2);"
"poly(o1);"
"poly(o2);"
......
......@@ -8,14 +8,14 @@
// This is to test for dictionary mode when there more than
// kMaxNumberOfDescriptors (1024) properties.
const kLimit = 1030;
let evalString = "(function(i) { " +
let evalString = "function f(i) { " +
"let clazz = class { " +
" constructor(i) { this.value = i;";
for (let i = 0; i < kLimit ; i++) {
evalString += "this.property"+i +" = "+i+"; "
}
evalString += "}};" +
" return (new clazz(i)); })";
" return (new clazz(i)); }; f;";
let fn = eval(evalString);
%PrepareFunctionForOptimization(fn);
......
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