1- // ===-------- LoopIdiomVectorize.cpp - Loop idiom recognition -- -----------===//
1+ // ===-------- LoopIdiomVectorize.cpp - Loop idiom vectorization -----------===//
22//
33// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
44// See https://llvm.org/LICENSE.txt for license information.
4040// ===----------------------------------------------------------------------===//
4141
4242#include " llvm/Transforms/Vectorize/LoopIdiomVectorize.h"
43- #include " llvm/ADT/ScopeExit.h"
4443#include " llvm/Analysis/DomTreeUpdater.h"
4544#include " llvm/Analysis/LoopPass.h"
4645#include " llvm/Analysis/TargetTransformInfo.h"
@@ -58,33 +57,27 @@ using namespace PatternMatch;
5857
5958static cl::opt<bool > DisableAll (" disable-loop-idiom-vectorize-all" , cl::Hidden,
6059 cl::init (false ),
61- cl::desc(" Disable Loop Idiom Transform Pass." ));
60+ cl::desc(" Disable Loop Idiom Vectorize Pass." ));
6261
6362static cl::opt<bool >
6463 DisableByteCmp (" disable-loop-idiom-vectorize-bytecmp" , cl::Hidden,
6564 cl::init (false ),
66- cl::desc(" Proceed with Loop Idiom Transform Pass, but do "
65+ cl::desc(" Proceed with Loop Idiom Vectorize Pass, but do "
6766 " not convert byte-compare loop(s)." ));
6867
6968static cl::opt<bool >
70- VerifyLoops (" verify- loop-idiom-vectorize" , cl::Hidden, cl::init(false ),
71- cl::desc(" Verify loops generated Loop Idiom Transform Pass." ));
69+ VerifyLoops (" loop-idiom-vectorize-verify " , cl::Hidden, cl::init(false ),
70+ cl::desc(" Verify loops generated Loop Idiom Vectorize Pass." ));
7271
7372namespace {
73+
7474class LoopIdiomVectorize {
7575 Loop *CurLoop = nullptr ;
7676 DominatorTree *DT;
7777 LoopInfo *LI;
7878 const TargetTransformInfo *TTI;
7979 const DataLayout *DL;
8080
81- // Blocks that will be used for inserting vectorized code.
82- BasicBlock *EndBlock = nullptr ;
83- BasicBlock *VectorLoopPreheaderBlock = nullptr ;
84- BasicBlock *VectorLoopStartBlock = nullptr ;
85- BasicBlock *VectorLoopMismatchBlock = nullptr ;
86- BasicBlock *VectorLoopIncBlock = nullptr ;
87-
8881public:
8982 explicit LoopIdiomVectorize (DominatorTree *DT, LoopInfo *LI,
9083 const TargetTransformInfo *TTI,
@@ -102,15 +95,9 @@ class LoopIdiomVectorize {
10295 SmallVectorImpl<BasicBlock *> &ExitBlocks);
10396
10497 bool recognizeByteCompare ();
105-
10698 Value *expandFindMismatch (IRBuilder<> &Builder, DomTreeUpdater &DTU,
10799 GetElementPtrInst *GEPA, GetElementPtrInst *GEPB,
108100 Instruction *Index, Value *Start, Value *MaxLen);
109-
110- Value *createMaskedFindMismatch (IRBuilder<> &Builder, GetElementPtrInst *GEPA,
111- GetElementPtrInst *GEPB, Value *ExtStart,
112- Value *ExtEnd);
113-
114101 void transformByteCompare (GetElementPtrInst *GEPA, GetElementPtrInst *GEPB,
115102 PHINode *IndPhi, Value *MaxLen, Instruction *Index,
116103 Value *Start, bool IncIdx, BasicBlock *FoundBB,
@@ -344,106 +331,6 @@ bool LoopIdiomVectorize::recognizeByteCompare() {
344331 return true ;
345332}
346333
347- Value *LoopIdiomVectorize::createMaskedFindMismatch (IRBuilder<> &Builder,
348- GetElementPtrInst *GEPA,
349- GetElementPtrInst *GEPB,
350- Value *ExtStart,
351- Value *ExtEnd) {
352- Type *I64Type = Builder.getInt64Ty ();
353- Type *ResType = Builder.getInt32Ty ();
354- Type *LoadType = Builder.getInt8Ty ();
355- Value *PtrA = GEPA->getPointerOperand ();
356- Value *PtrB = GEPB->getPointerOperand ();
357-
358- // At this point we know two things must be true:
359- // 1. Start <= End
360- // 2. ExtMaxLen <= MinPageSize due to the page checks.
361- // Therefore, we know that we can use a 64-bit induction variable that
362- // starts from 0 -> ExtMaxLen and it will not overflow.
363- ScalableVectorType *PredVTy =
364- ScalableVectorType::get (Builder.getInt1Ty (), 16 );
365-
366- Value *InitialPred = Builder.CreateIntrinsic (
367- Intrinsic::get_active_lane_mask, {PredVTy, I64Type}, {ExtStart, ExtEnd});
368-
369- Value *VecLen = Builder.CreateIntrinsic (Intrinsic::vscale, {I64Type}, {});
370- VecLen = Builder.CreateMul (VecLen, ConstantInt::get (I64Type, 16 ), " " ,
371- /* HasNUW=*/ true , /* HasNSW=*/ true );
372-
373- Value *PFalse = Builder.CreateVectorSplat (PredVTy->getElementCount (),
374- Builder.getInt1 (false ));
375-
376- BranchInst *JumpToVectorLoop = BranchInst::Create (VectorLoopStartBlock);
377- Builder.Insert (JumpToVectorLoop);
378-
379- // Set up the first vector loop block by creating the PHIs, doing the vector
380- // loads and comparing the vectors.
381- Builder.SetInsertPoint (VectorLoopStartBlock);
382- PHINode *LoopPred = Builder.CreatePHI (PredVTy, 2 , " mismatch_vec_loop_pred" );
383- LoopPred->addIncoming (InitialPred, VectorLoopPreheaderBlock);
384- PHINode *VectorIndexPhi = Builder.CreatePHI (I64Type, 2 , " mismatch_vec_index" );
385- VectorIndexPhi->addIncoming (ExtStart, VectorLoopPreheaderBlock);
386- Type *VectorLoadType = ScalableVectorType::get (Builder.getInt8Ty (), 16 );
387- Value *Passthru = ConstantInt::getNullValue (VectorLoadType);
388-
389- Value *VectorLhsGep =
390- Builder.CreateGEP (LoadType, PtrA, VectorIndexPhi, " " , GEPA->isInBounds ());
391- Value *VectorLhsLoad = Builder.CreateMaskedLoad (VectorLoadType, VectorLhsGep,
392- Align (1 ), LoopPred, Passthru);
393-
394- Value *VectorRhsGep =
395- Builder.CreateGEP (LoadType, PtrB, VectorIndexPhi, " " , GEPB->isInBounds ());
396- Value *VectorRhsLoad = Builder.CreateMaskedLoad (VectorLoadType, VectorRhsGep,
397- Align (1 ), LoopPred, Passthru);
398-
399- Value *VectorMatchCmp = Builder.CreateICmpNE (VectorLhsLoad, VectorRhsLoad);
400- VectorMatchCmp = Builder.CreateSelect (LoopPred, VectorMatchCmp, PFalse);
401- Value *VectorMatchHasActiveLanes = Builder.CreateOrReduce (VectorMatchCmp);
402- BranchInst *VectorEarlyExit = BranchInst::Create (
403- VectorLoopMismatchBlock, VectorLoopIncBlock, VectorMatchHasActiveLanes);
404- Builder.Insert (VectorEarlyExit);
405-
406- // Increment the index counter and calculate the predicate for the next
407- // iteration of the loop. We branch back to the start of the loop if there
408- // is at least one active lane.
409- Builder.SetInsertPoint (VectorLoopIncBlock);
410- Value *NewVectorIndexPhi =
411- Builder.CreateAdd (VectorIndexPhi, VecLen, " " ,
412- /* HasNUW=*/ true , /* HasNSW=*/ true );
413- VectorIndexPhi->addIncoming (NewVectorIndexPhi, VectorLoopIncBlock);
414- Value *NewPred =
415- Builder.CreateIntrinsic (Intrinsic::get_active_lane_mask,
416- {PredVTy, I64Type}, {NewVectorIndexPhi, ExtEnd});
417- LoopPred->addIncoming (NewPred, VectorLoopIncBlock);
418-
419- Value *PredHasActiveLanes =
420- Builder.CreateExtractElement (NewPred, uint64_t (0 ));
421- BranchInst *VectorLoopBranchBack =
422- BranchInst::Create (VectorLoopStartBlock, EndBlock, PredHasActiveLanes);
423- Builder.Insert (VectorLoopBranchBack);
424-
425- // If we found a mismatch then we need to calculate which lane in the vector
426- // had a mismatch and add that on to the current loop index.
427- Builder.SetInsertPoint (VectorLoopMismatchBlock);
428- PHINode *FoundPred = Builder.CreatePHI (PredVTy, 1 , " mismatch_vec_found_pred" );
429- FoundPred->addIncoming (VectorMatchCmp, VectorLoopStartBlock);
430- PHINode *LastLoopPred =
431- Builder.CreatePHI (PredVTy, 1 , " mismatch_vec_last_loop_pred" );
432- LastLoopPred->addIncoming (LoopPred, VectorLoopStartBlock);
433- PHINode *VectorFoundIndex =
434- Builder.CreatePHI (I64Type, 1 , " mismatch_vec_found_index" );
435- VectorFoundIndex->addIncoming (VectorIndexPhi, VectorLoopStartBlock);
436-
437- Value *PredMatchCmp = Builder.CreateAnd (LastLoopPred, FoundPred);
438- Value *Ctz = Builder.CreateIntrinsic (
439- Intrinsic::experimental_cttz_elts, {ResType, PredMatchCmp->getType ()},
440- {PredMatchCmp, /* ZeroIsPoison=*/ Builder.getInt1 (true )});
441- Ctz = Builder.CreateZExt (Ctz, I64Type);
442- Value *VectorLoopRes64 = Builder.CreateAdd (VectorFoundIndex, Ctz, " " ,
443- /* HasNUW=*/ true , /* HasNSW=*/ true );
444- return Builder.CreateTrunc (VectorLoopRes64, ResType);
445- }
446-
447334Value *LoopIdiomVectorize::expandFindMismatch (
448335 IRBuilder<> &Builder, DomTreeUpdater &DTU, GetElementPtrInst *GEPA,
449336 GetElementPtrInst *GEPB, Instruction *Index, Value *Start, Value *MaxLen) {
@@ -458,7 +345,8 @@ Value *LoopIdiomVectorize::expandFindMismatch(
458345 Type *ResType = Builder.getInt32Ty ();
459346
460347 // Split block in the original loop preheader.
461- EndBlock = SplitBlock (Preheader, PHBranch, DT, LI, nullptr , " mismatch_end" );
348+ BasicBlock *EndBlock =
349+ SplitBlock (Preheader, PHBranch, DT, LI, nullptr , " mismatch_end" );
462350
463351 // Create the blocks that we're going to need:
464352 // 1. A block for checking the zero-extended length exceeds 0
@@ -482,17 +370,17 @@ Value *LoopIdiomVectorize::expandFindMismatch(
482370 BasicBlock *MemCheckBlock = BasicBlock::Create (
483371 Ctx, " mismatch_mem_check" , EndBlock->getParent (), EndBlock);
484372
485- VectorLoopPreheaderBlock = BasicBlock::Create (
373+ BasicBlock * VectorLoopPreheaderBlock = BasicBlock::Create (
486374 Ctx, " mismatch_vec_loop_preheader" , EndBlock->getParent (), EndBlock);
487375
488- VectorLoopStartBlock = BasicBlock::Create (Ctx, " mismatch_vec_loop " ,
489- EndBlock->getParent (), EndBlock);
376+ BasicBlock * VectorLoopStartBlock = BasicBlock::Create (
377+ Ctx, " mismatch_vec_loop " , EndBlock->getParent (), EndBlock);
490378
491- VectorLoopIncBlock = BasicBlock::Create (Ctx, " mismatch_vec_loop_inc " ,
492- EndBlock->getParent (), EndBlock);
379+ BasicBlock * VectorLoopIncBlock = BasicBlock::Create (
380+ Ctx, " mismatch_vec_loop_inc " , EndBlock->getParent (), EndBlock);
493381
494- VectorLoopMismatchBlock = BasicBlock::Create (Ctx, " mismatch_vec_loop_found " ,
495- EndBlock->getParent (), EndBlock);
382+ BasicBlock * VectorLoopMismatchBlock = BasicBlock::Create (
383+ Ctx, " mismatch_vec_loop_found " , EndBlock->getParent (), EndBlock);
496384
497385 BasicBlock *LoopPreHeaderBlock = BasicBlock::Create (
498386 Ctx, " mismatch_loop_pre" , EndBlock->getParent (), EndBlock);
@@ -548,6 +436,10 @@ Value *LoopIdiomVectorize::expandFindMismatch(
548436 MDBuilder (MinItCheckBr->getContext ()).createBranchWeights (99 , 1 ));
549437 Builder.Insert (MinItCheckBr);
550438
439+ DTU.applyUpdates (
440+ {{DominatorTree::Insert, MinItCheckBlock, MemCheckBlock},
441+ {DominatorTree::Insert, MinItCheckBlock, LoopPreHeaderBlock}});
442+
551443 // For each of the arrays, check the start/end addresses are on the same
552444 // page.
553445 Builder.SetInsertPoint (MemCheckBlock);
@@ -590,20 +482,126 @@ Value *LoopIdiomVectorize::expandFindMismatch(
590482 .createBranchWeights (10 , 90 ));
591483 Builder.Insert (CombinedPageCmpCmpBr);
592484
485+ DTU.applyUpdates (
486+ {{DominatorTree::Insert, MemCheckBlock, LoopPreHeaderBlock},
487+ {DominatorTree::Insert, MemCheckBlock, VectorLoopPreheaderBlock}});
488+
593489 // Set up the vector loop preheader, i.e. calculate initial loop predicate,
594490 // zero-extend MaxLen to 64-bits, determine the number of vector elements
595491 // processed in each iteration, etc.
596492 Builder.SetInsertPoint (VectorLoopPreheaderBlock);
597493
598- Value *VectorLoopRes =
599- createMaskedFindMismatch (Builder, GEPA, GEPB, ExtStart, ExtEnd);
494+ // At this point we know two things must be true:
495+ // 1. Start <= End
496+ // 2. ExtMaxLen <= MinPageSize due to the page checks.
497+ // Therefore, we know that we can use a 64-bit induction variable that
498+ // starts from 0 -> ExtMaxLen and it will not overflow.
499+ ScalableVectorType *PredVTy =
500+ ScalableVectorType::get (Builder.getInt1Ty (), 16 );
501+
502+ Value *InitialPred = Builder.CreateIntrinsic (
503+ Intrinsic::get_active_lane_mask, {PredVTy, I64Type}, {ExtStart, ExtEnd});
504+
505+ Value *VecLen = Builder.CreateIntrinsic (Intrinsic::vscale, {I64Type}, {});
506+ VecLen = Builder.CreateMul (VecLen, ConstantInt::get (I64Type, 16 ), " " ,
507+ /* HasNUW=*/ true , /* HasNSW=*/ true );
508+
509+ Value *PFalse = Builder.CreateVectorSplat (PredVTy->getElementCount (),
510+ Builder.getInt1 (false ));
511+
512+ BranchInst *JumpToVectorLoop = BranchInst::Create (VectorLoopStartBlock);
513+ Builder.Insert (JumpToVectorLoop);
514+
515+ DTU.applyUpdates ({{DominatorTree::Insert, VectorLoopPreheaderBlock,
516+ VectorLoopStartBlock}});
517+
518+ // Set up the first vector loop block by creating the PHIs, doing the vector
519+ // loads and comparing the vectors.
520+ Builder.SetInsertPoint (VectorLoopStartBlock);
521+ PHINode *LoopPred = Builder.CreatePHI (PredVTy, 2 , " mismatch_vec_loop_pred" );
522+ LoopPred->addIncoming (InitialPred, VectorLoopPreheaderBlock);
523+ PHINode *VectorIndexPhi = Builder.CreatePHI (I64Type, 2 , " mismatch_vec_index" );
524+ VectorIndexPhi->addIncoming (ExtStart, VectorLoopPreheaderBlock);
525+ Type *VectorLoadType = ScalableVectorType::get (Builder.getInt8Ty (), 16 );
526+ Value *Passthru = ConstantInt::getNullValue (VectorLoadType);
527+
528+ Value *VectorLhsGep =
529+ Builder.CreateGEP (LoadType, PtrA, VectorIndexPhi, " " , GEPA->isInBounds ());
530+ Value *VectorLhsLoad = Builder.CreateMaskedLoad (VectorLoadType, VectorLhsGep,
531+ Align (1 ), LoopPred, Passthru);
532+
533+ Value *VectorRhsGep =
534+ Builder.CreateGEP (LoadType, PtrB, VectorIndexPhi, " " , GEPB->isInBounds ());
535+ Value *VectorRhsLoad = Builder.CreateMaskedLoad (VectorLoadType, VectorRhsGep,
536+ Align (1 ), LoopPred, Passthru);
537+
538+ Value *VectorMatchCmp = Builder.CreateICmpNE (VectorLhsLoad, VectorRhsLoad);
539+ VectorMatchCmp = Builder.CreateSelect (LoopPred, VectorMatchCmp, PFalse);
540+ Value *VectorMatchHasActiveLanes = Builder.CreateOrReduce (VectorMatchCmp);
541+ BranchInst *VectorEarlyExit = BranchInst::Create (
542+ VectorLoopMismatchBlock, VectorLoopIncBlock, VectorMatchHasActiveLanes);
543+ Builder.Insert (VectorEarlyExit);
544+
545+ DTU.applyUpdates (
546+ {{DominatorTree::Insert, VectorLoopStartBlock, VectorLoopMismatchBlock},
547+ {DominatorTree::Insert, VectorLoopStartBlock, VectorLoopIncBlock}});
548+
549+ // Increment the index counter and calculate the predicate for the next
550+ // iteration of the loop. We branch back to the start of the loop if there
551+ // is at least one active lane.
552+ Builder.SetInsertPoint (VectorLoopIncBlock);
553+ Value *NewVectorIndexPhi =
554+ Builder.CreateAdd (VectorIndexPhi, VecLen, " " ,
555+ /* HasNUW=*/ true , /* HasNSW=*/ true );
556+ VectorIndexPhi->addIncoming (NewVectorIndexPhi, VectorLoopIncBlock);
557+ Value *NewPred =
558+ Builder.CreateIntrinsic (Intrinsic::get_active_lane_mask,
559+ {PredVTy, I64Type}, {NewVectorIndexPhi, ExtEnd});
560+ LoopPred->addIncoming (NewPred, VectorLoopIncBlock);
561+
562+ Value *PredHasActiveLanes =
563+ Builder.CreateExtractElement (NewPred, uint64_t (0 ));
564+ BranchInst *VectorLoopBranchBack =
565+ BranchInst::Create (VectorLoopStartBlock, EndBlock, PredHasActiveLanes);
566+ Builder.Insert (VectorLoopBranchBack);
567+
568+ DTU.applyUpdates (
569+ {{DominatorTree::Insert, VectorLoopIncBlock, VectorLoopStartBlock},
570+ {DominatorTree::Insert, VectorLoopIncBlock, EndBlock}});
571+
572+ // If we found a mismatch then we need to calculate which lane in the vector
573+ // had a mismatch and add that on to the current loop index.
574+ Builder.SetInsertPoint (VectorLoopMismatchBlock);
575+ PHINode *FoundPred = Builder.CreatePHI (PredVTy, 1 , " mismatch_vec_found_pred" );
576+ FoundPred->addIncoming (VectorMatchCmp, VectorLoopStartBlock);
577+ PHINode *LastLoopPred =
578+ Builder.CreatePHI (PredVTy, 1 , " mismatch_vec_last_loop_pred" );
579+ LastLoopPred->addIncoming (LoopPred, VectorLoopStartBlock);
580+ PHINode *VectorFoundIndex =
581+ Builder.CreatePHI (I64Type, 1 , " mismatch_vec_found_index" );
582+ VectorFoundIndex->addIncoming (VectorIndexPhi, VectorLoopStartBlock);
583+
584+ Value *PredMatchCmp = Builder.CreateAnd (LastLoopPred, FoundPred);
585+ Value *Ctz = Builder.CreateIntrinsic (
586+ Intrinsic::experimental_cttz_elts, {ResType, PredMatchCmp->getType ()},
587+ {PredMatchCmp, /* ZeroIsPoison=*/ Builder.getInt1 (true )});
588+ Ctz = Builder.CreateZExt (Ctz, I64Type);
589+ Value *VectorLoopRes64 = Builder.CreateAdd (VectorFoundIndex, Ctz, " " ,
590+ /* HasNUW=*/ true , /* HasNSW=*/ true );
591+ Value *VectorLoopRes = Builder.CreateTrunc (VectorLoopRes64, ResType);
600592
601593 Builder.Insert (BranchInst::Create (EndBlock));
602594
595+ DTU.applyUpdates (
596+ {{DominatorTree::Insert, VectorLoopMismatchBlock, EndBlock}});
597+
603598 // Generate code for scalar loop.
604599 Builder.SetInsertPoint (LoopPreHeaderBlock);
605600 Builder.Insert (BranchInst::Create (LoopStartBlock));
606601
602+ DTU.applyUpdates (
603+ {{DominatorTree::Insert, LoopPreHeaderBlock, LoopStartBlock}});
604+
607605 Builder.SetInsertPoint (LoopStartBlock);
608606 PHINode *IndexPhi = Builder.CreatePHI (ResType, 2 , " mismatch_index" );
609607 IndexPhi->addIncoming (Start, LoopPreHeaderBlock);
@@ -625,6 +623,9 @@ Value *LoopIdiomVectorize::expandFindMismatch(
625623 BranchInst *MatchCmpBr = BranchInst::Create (LoopIncBlock, EndBlock, MatchCmp);
626624 Builder.Insert (MatchCmpBr);
627625
626+ DTU.applyUpdates ({{DominatorTree::Insert, LoopStartBlock, LoopIncBlock},
627+ {DominatorTree::Insert, LoopStartBlock, EndBlock}});
628+
628629 // Have we reached the maximum permitted length for the loop?
629630 Builder.SetInsertPoint (LoopIncBlock);
630631 Value *PhiInc = Builder.CreateAdd (IndexPhi, ConstantInt::get (ResType, 1 ), " " ,
@@ -635,6 +636,9 @@ Value *LoopIdiomVectorize::expandFindMismatch(
635636 BranchInst *IVCmpBr = BranchInst::Create (EndBlock, LoopStartBlock, IVCmp);
636637 Builder.Insert (IVCmpBr);
637638
639+ DTU.applyUpdates ({{DominatorTree::Insert, LoopIncBlock, EndBlock},
640+ {DominatorTree::Insert, LoopIncBlock, LoopStartBlock}});
641+
638642 // In the end block we need to insert a PHI node to deal with three cases:
639643 // 1. We didn't find a mismatch in the scalar loop, so we return MaxLen.
640644 // 2. We exitted the scalar loop early due to a mismatch and need to return
@@ -678,11 +682,6 @@ void LoopIdiomVectorize::transformByteCompare(GetElementPtrInst *GEPA,
678682 DomTreeUpdater DTU (DT, DomTreeUpdater::UpdateStrategy::Lazy);
679683 Builder.SetCurrentDebugLocation (PHBranch->getDebugLoc ());
680684
681- // Safeguard to check if we build the correct DomTree with DTU.
682- auto CheckDTU = llvm::make_scope_exit ([&]() {
683- assert (DTU.getDomTree ().verify () && " Ill-formed DomTree built by DTU" );
684- });
685-
686685 // Increment the pointer if this was done before the loads in the loop.
687686 if (IncIdx)
688687 Start = Builder.CreateAdd (Start, ConstantInt::get (Start->getType (), 1 ));
@@ -718,8 +717,12 @@ void LoopIdiomVectorize::transformByteCompare(GetElementPtrInst *GEPA,
718717 if (FoundBB != EndBB) {
719718 Value *FoundCmp = Builder.CreateICmpEQ (ByteCmpRes, MaxLen);
720719 Builder.CreateCondBr (FoundCmp, EndBB, FoundBB);
720+ DTU.applyUpdates ({{DominatorTree::Insert, CmpBB, FoundBB},
721+ {DominatorTree::Insert, CmpBB, EndBB}});
722+
721723 } else {
722724 Builder.CreateBr (FoundBB);
725+ DTU.applyUpdates ({{DominatorTree::Insert, CmpBB, FoundBB}});
723726 }
724727
725728 auto fixSuccessorPhis = [&](BasicBlock *SuccBB) {
0 commit comments