This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][CostModel] Detects that {Extract,Insert}Element at lane 0 have the same cost as other lanes for real instructions that operates on integer types
ClosedPublic

Authored by mingmingl on Jun 21 2022, 1:38 PM.

Details

Summary

Currently, {extract,insert}-element has zero cost at lane 0 [1]. However, there is a cost (by fmov instruction at lane 0 [2], or more generally ext/ins instruction) to move values from SIMD registers to GPR registers, when the element is used explicitly as integers.

See https://godbolt.org/z/faPE1nTn8, when fmov is generated for d* register -> x* register conversion.

Implementation-wise, add a private method AArch64TTIImpl::getVectorInstrCostHelper as a helper function. This way, instruction-based method could share the core logic (e.g., returning zero cost if type is legalized to scalar).

[1] https://github.com/llvm/llvm-project/blob/2cf320d41ed708679e01eeeb93f58d6c5c88ba7a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp#L1853
[2] https://github.com/llvm/llvm-project/blob/2cf320d41ed708679e01eeeb93f58d6c5c88ba7a/llvm/lib/Target/AArch64/AArch64InstrInfo.td#L8150-L8157

Diff Detail

Event Timeline

mingmingl created this revision.Jun 21 2022, 1:38 PM
mingmingl requested review of this revision.Jun 21 2022, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 1:38 PM
mingmingl edited the summary of this revision. (Show Details)Jun 21 2022, 1:50 PM
mingmingl edited the summary of this revision. (Show Details)Jun 21 2022, 1:53 PM
mingmingl added reviewers: fhahn, nikic, davidxl.
nikic requested changes to this revision.Jun 21 2022, 1:57 PM

Changing the cost model (even if it impacts other places) is the right thing to do here. We cannot accept such special cases in LICM.

This revision now requires changes to proceed.Jun 21 2022, 1:57 PM

Changing the cost model (even if it impacts other places) is the right thing to do here. We cannot accept such special cases in LICM.

Acknowledged and it makes sense to me. Is it more idiomatic to create a new revision (to update the cost model) or just modify this existing revision?

Keeping this revision with discussion history in one place sounds fine.

mingmingl updated this revision to Diff 440670.Jun 28 2022, 9:58 AM
mingmingl retitled this revision from [LICM][AArch64] Detects that ExtractElement is not free in AArch64 when deciding whether to sink an operation in the exiting block of a loop. to [AArch64][CostModel] Detects that ExtractElement at index is not free in AArch64 when result is used as integer..
mingmingl edited the summary of this revision. (Show Details)

For getVectorInstrCost method, optionally pass a pointer of Instruction, and each target can retrieve the (use) context and customize cost estimation. For AArch64 implementation, fall back to use VectorInsertExtractBaseCost rather than regard extract element at index 0 as a free operation.

davidxl added inline comments.Jun 28 2022, 10:39 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1879

missing return instruction ?

mingmingl marked an inline comment as done.Jun 28 2022, 11:18 AM
mingmingl added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1879

This falls through to use ST->getVectorInsertExtractBaseCost() around line 1903. Basically, this patch reduces the case when extracting element at index 0 is considered free (using truck as a baseline).

davidxl added inline comments.Jun 28 2022, 11:29 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1879

ok -- my view of the patch hides the line :)

nikic resigned from this revision.Jun 28 2022, 12:13 PM

(Not really familiar with cost model APIs.)

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Yes, all users of an instruction are instructions, it's safe to use cast here.

1879

This should probably be just U.getOperandNo() == 0?

mingmingl updated this revision to Diff 440759.Jun 28 2022, 1:53 PM
mingmingl marked an inline comment as done.

In AArch64TargetTransformInfo.cpp, change helper function isExtractedElementUsedAsInteger by

  1. assert that all users of an instruction are instructions
  2. simplify the store operation usage check, since integer could only be directly used as value as it is, the source of store operation (i.e., not the destination to store the value to).
  3. require that uitofpinst/sitofpinst is the only use of extract-element if we think the extract-element operation is free.
mingmingl marked 2 inline comments as done.Jun 28 2022, 1:56 PM
mingmingl added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

thanks for taking a look! I looked at a few existing cpp and they also assume so.

Changing this to an assert then.

1879

I meant to check that the extract-element is used as the value, the source of store operation.

However, upon this comment, I realize that the extracted element could only be the source, since the destination should be a pointer type. Simplified the store operation check.

mingmingl updated this revision to Diff 440763.Jun 28 2022, 2:04 PM
mingmingl marked 2 inline comments as done.

Change to AArch64TargetTransformInfo.cpp only:

  1. Add the missing continue for store operation check.
  2. Remove the OneUse restriction added for uitofpinst/sitofpinst.

Hm, do we have precedence for doing a full scan of users in other cost APIs?

RKSimon added inline comments.Jun 30 2022, 8:29 AM
llvm/include/llvm/CodeGen/BasicTTIImpl.h
92

any chance we can add a default (nullptr) arg value and avoid all the nullptr additions?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1859

!isa_and_nonnull<ExtractElementInst>(Inst) ?

1868

Scanning the uses like this isn't going to be cheap - and getCost calls can be called a LOT.

Why did you decide this was necessary? Do existing tests change without it? You don't appear to have added any test coverage for these cases.

llvm/test/Analysis/CostModel/AArch64/kryo.ll
37

Add the other CHECKs for other extraction indices?

43

pre-commit the test with trunk codegen and rebase the patch to show the change in cost reporting

mingmingl marked 2 inline comments as done.Jun 30 2022, 12:50 PM
mingmingl added inline comments.
llvm/include/llvm/CodeGen/BasicTTIImpl.h
92

This is a good question.

If we want to have a default (nullptr) arg, each derived class (AArch64TargetTransformInfo, X86TargetTransformInfo, etc) needs to override the default pointer argument, since derived classes won't inherit the default argument of base class's virtual method [1] and it's suggested not to "change the default parameters of overridden inherited functions" [2].

I don't feel strong about this, but not adding a default following the existing style of Index -> Index has a default value of -1 in class TargetTransformInfo, but not in class BasicTTIImpl or derived classes.

May I know your thoughts about this? (leaving this comment open)

[1] https://stackoverflow.com/questions/3533589/can-virtual-functions-have-default-parameters
[2] http://www.gotw.ca/gotw/005.htm

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Scanning cost is a good point; one mitigation (in the patch) is that, there is a break out of the loop once an integer use case if ound.

May I measure the compile time increase with https://github.com/nikic/llvm-compile-time-tracker and report back? (leaving this open)

llvm/test/Analysis/CostModel/AArch64/kryo.ll
43

will do in another patch (got to reply first for compile-time question around scanning uses())

Changes

  1. Update commit message to mirror phabricator summary.
  2. Use`!isa_and_null`, and added the other CHECKs for other extraction indices, as suggested.
mingmingl marked an inline comment as done.Jun 30 2022, 2:21 PM
mingmingl added inline comments.
llvm/test/Analysis/CostModel/AArch64/kryo.ll
43

created https://reviews.llvm.org/D128945, with this test case, and another test case in LICM/AArch64/ subdirectory.

Both test cases are going to have diff, and the diff for LICM/AArch64/extract-element.ll would be like https://www.diffchecker.com/RyiE4Zn5 (code of trunk on the left, code of this patch on the right)

mingmingl added inline comments.Jun 30 2022, 3:05 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Compared between HEAD~1 and this patch gives https://llvm-compile-time-tracker.com/compare.php?from=1e556f459b44dd0ca4073e932f66ecb6f40fe31a&to=63035f714c94d31698b5b4ab79b9912814c29345&stat=instructions

Across different configs and benchmarks, the compile-time increases or decreases within +/- 0.1%. I wonder if this means it's fine in terms of the compile-time cost?

mingmingl added inline comments.Jun 30 2022, 4:25 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Actually I wasn't sure if AArch64 code path is executed at this point (without reading how tracker is implemented) even if the metrics (e.g., instructions) could be collected without running the code. Do you have other suggestions to measure the cost of iterating uses?

Why did you decide this was necessary?

In order to know if there is a scalar integer use (that results in additional operations to move / dup vector subregisters to general purpose registers), all uses of the extract-element instruction are enumerated here.

Do existing tests change without it?

The reason to handle store is based on this example (https://godbolt.org/z/fn4jWr5eW).

You don't appear to have added any test coverage for these cases.

I could add https://godbolt.org/z/fn4jWr5eW as a test case if it sounds good.

mingmingl added inline comments.Jul 6 2022, 4:04 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Actually I wasn't sure if AArch64 code path is executed at this point (without reading how tracker is implemented) even if the metrics (e.g., instructions) could be collected without running the code. Do you have other suggestions to measure the cost of iterating uses?

Regarding compile-time change

  1. The llvm-compile-time-tracker measures x86 code paths.
  2. I did a similar comparison (by scraping scripts) for -DLLVM_TARGETS_TO_BUILD="AArch64" , and build llvm-test-suite with O3
    1. The diff of instruction counts are within +/- 0.1%, and the diff of cycles changes are within 2% (CTMark results uploaded )

Regarding the pattern of iterating uses() and its cycle change

  • I think it's not un-common for a pass to iterate uses() of instructions, and here only a subset of extract-element instructions uses are iterated.
  • On the other hand, "used-as-integer" fact might be pre-computed and maintained inside class ExtractElementInst, so AArch64TTI can query / re-use the result, if we do want to avoid calculating it repeatedly.

Would like to hear some thoughts for the topics above, thanks!

mingmingl added inline comments.Jul 6 2022, 9:59 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

Between iterating all uses and not looking at uses, another option for now is to look at use_empty(), since the instruction cost would ultimately be 0 (regardless of index value) if use_empty() returns true.

Together with this option, a FIXME might be added to point out if all uses are store operations that could be performed bitwise (i.e., without cross-register-bank copies), the extract-element operation is free.

mingmingl updated this revision to Diff 443781.Jul 11 2022, 3:54 PM
mingmingl marked an inline comment as done.

Address concerns about scanning uses in cost API.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1868

To summarize the question and solution:

Since no full-scan of use exists due to compile-time concerns, simplify the helper function to look at 'use_empty' only, and added a FIXME for store operations.

I want to suggest a different way of changing the interfaces to make the intention clearer:

  1. add a new interface for vectorcost in TTI that takes an instruction (to replace the opcode parameter)
  2. for places where context instruction is not available, no changes are needed;
  3. in TTIImpl, by default, the instruction based interface is simply a wrapper to the opcode based interface
  4. for aarch64, additional logic is added for checking the lane ...
mingmingl marked an inline comment as done.

Address the comments by an interface change.

davidxl added inline comments.Jul 13 2022, 12:00 PM
llvm/include/llvm/Analysis/TargetTransformInfo.h
1155

Perhaps document this that this is used when the instruction is not available.

1164

Mention that this interface is used when I is available (and the interface implementation asserts it is not nullptr).

llvm/lib/Analysis/TargetTransformInfo.cpp
866

probably submit this as a NFC patch seperately?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
176

what is the purpose of the using statement here?

mingmingl marked 4 inline comments as done.

Address comments.

llvm/include/llvm/Analysis/TargetTransformInfo.h
1155

Done. Mention the typical use as provision vectorization/scalarization cost in vectorizer passes.

llvm/lib/Analysis/TargetTransformInfo.cpp
866

Sure. Will leave the FIXME inside this function, and the assert the newly added 'getVectorInstrCost'.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
176

Oh good catch. This is a mistake. 'using statement' is not needed for AArch64 since there is an override for new interface.

davidxl added inline comments.Jul 13 2022, 1:13 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
176

Is it needed for other targets? The new method is a public method in BasicTTIImplBase, which is inherited and visible in the derived class.

mingmingl marked an inline comment as done.Jul 13 2022, 1:28 PM
mingmingl added inline comments.
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
176

Is it needed for other targets?

Yes, and I learnt it by seeing the compile error without 'using statements'.

Basically, the overloaded method in derived class could hide method in the base class, even if function signature is different. There is one example, and one relevant discussion.

Also, renaming the new interface differently also compiles, since name hiding doesn't exist by renaming.

There are using BaseT::getIntImmCost statements in <Target>TargetTransformInfo.cpp (example) (not sure if 'getIntImmCost' still helps with name hiding now, since overtime many targets overload the method with the same function signature)

Since the use scanning code is removed (which raised some compile time concern), and latest revisions looks good to me. Please wait a little for other reviewers to comment.

mingmingl marked an inline comment as done.Jul 25 2022, 3:05 PM

ping.

Matt added a subscriber: Matt.Aug 2 2022, 8:18 PM

Consider split this into two patches: the first NFC just introduces the new interface (as a simple wrapper to the existing opcode based one). The second one is the functional change.

Consider split this into two patches: the first NFC just introduces the new interface (as a simple wrapper to the existing opcode based one). The second one is the functional change.

Sent out D131114 as NFC.

mingmingl updated this revision to Diff 450143.Aug 4 2022, 3:01 PM

Rebase after NFC.

RKSimon added inline comments.Aug 11 2022, 3:59 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1887

do you need the this-> ?

mingmingl marked an inline comment as done.

resolve comments.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1887

Ah this-> is not needed, fixed it.

(Sorry for the delay; I was out in the past week)

LGTM but a aarch64 specialist should sign off as well

fhahn added a comment.Aug 16 2022, 1:31 AM

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1887

nit: LLVM coding style uses UpperCase for variables.

1993

I might be missing something here, but the comment here doesn't seem to match the code, which just checks if the result is used. I'd expect that in all interesting cases the result will be used. And if it is not used the cost is not really that interesting, as it is effectively a no-op that should be removed.

mingmingl added inline comments.Aug 17 2022, 10:37 PM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1993

It makes complete sense that instructions without users are not interesting, and [1] is sufficient for this patch (will hold on updating diff for better context). Let me elaborate why this patch and it'd be great to hear your thoughts! (This is a lot of words despite efforts to simplify them.. hopefully structure / bold fonts makes it easier. Thanks for reviewing this patch and reading the replies!)

About the motivating use case

  • It is shown in LICM/AArch64/extract-element.ll -> an extract-element instruction shouldn't be considered free (and thereby sinked to loop exit) by LICM when cost kind is TCK_SizeAndLatency -> extract-element has size and latency cost, unless it could be "combined" into its user instructions (e.g., user instruction can access lane directly).
  • Say the loop-exit is frequently executed in a hot path, sink extract-element (along with a series of its def instructions) is inefficient.
  • Redundancy elimination might be an alternative option but I haven't got a chance to look into the alternative option yet, with the thought that fixing cost model is overall a good change as well :)*

About the motivation of this patch

  • When the instruction exists in IR (indicated by function parameter const Instruction& I) and extract result is integer (a stronger signal of a fmov being emitted), this patch use the same cost all lanes (not considering lane 0 free).

So why this patch wants to tell a real vector instruction from a virtual one (by asking for const Instruction&I, not just Opcode)?

  • Vectorizer passes have Opcode but not real instructions in its provisioning stage [2]. Not telling real from virtual may make vectorizer result better or worse in a fragile way.
  • Plus, a new interface of getVectorInstCost won't mess up the codebase (w.r.t API/code style) when improvements for vectorizer cost models are added in the future.

But why vectorizer gives better or worse result in a fragile way?

  • Take BaseTTIImpl::getScalarizationOverhead as an example, it calls getVectorInstrCost element-by-element for virtual instructions. With various SIMD instructions, element-by-element estimation is not accurate for AArch64.
  • AArch64TTI doesn't override this method so BaseTTIImpl is used. Just as a reference, x86 backend overrides this method for some specialized cases.
  • If this patch is NOT no-op for method getScalarizationOverhead, it could exacerbates the inaccurate cost model (SLP, etc) and misleads vectorizers.
  • In practice, when non-zero cost for lane 0 is applied for "provisioned" vector instructions, some IR from llvm/test/Transforms/<vectorizer-subdir>/AArch64/ gets better generated code and some gets worse generated code. I didn't inspect them all, but presumably there are going to be better or worse cases, since without this change vectorizers sometimes gives worse output (e.g., https://godbolt.org/z/c1bjhGdT5 is for llvm/test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll)

A related question, I wonder if it's with a reason (or just unintended status quo) that getVectorInstrCost interface doesn't ask for a CostKind parameter?

[1]

+InstructionCost AArch64TTIImpl::getVectorInstrCost(const Instruction &I,
+                                                   Type *Val, unsigned Index) {
+  InstructionCost cost = getVectorInstrCost(I.getOpcode(), Val, Index);
+
+  // According to NEON programmer guide, other than multiply instructions,
+  // instructions that access scalars can access any element in the vector register.
+  //
+  // The cost of an extract depends on whether the user instruction.
+  // 1. If users could use scalars in vector registers directly (e.g., store), the
+  // extract-element operation is essentially free.
+  // 2. If the user instruction requires core register as operand (i.e.,
+  // cannot use scalars in vector register), an explicit move operation will
+  // be codegen'd.
+  //
+  // 'cost' might be an optimistic 0 when lane is 0.
+  // Returns the base cost (as the same with other lanes) if the scalar is an integer.
+  //
+  // FIXME:
+  // The cost of extract-element from vector is better estimated with context (e.g. users).
+  if (!isa_and_nonnull<ExtractElementInst>(&I) || !Val->getScalarType()->isIntegerTy()) 
+    return ST->getVectorInsertExtractBaseCost();
+  return cost;
+}
+

[2] Basically, vectorizer pass sees one form (scalar or vector) of "physical" instructions (that do exists in the IR), and tries to "provision" the cost of the other form; this means instructions of the other form doesn't exist yet, and only Opcode is known and used to provision the cost.

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Yeah I will remember to do this from now on (already did it earlier this week upon suggestions from another reviewer, and a partial diff was purely unintentional.) Also will hold on updating the patch to keep the patch context for discussion (leave all comments open).

I guess fhahn@ meant that for all useful scenarios, the instruction needs to have some uses (otherwise they are dead). For this case, the check should probably be tightened : instead of checking if the instruction has uses, it needs to examine the uses and differentiate integer uses that requires a fmov and other use contexts (which makes the extract noop).

Checking use emptiness can pessimize cases where the cost is zero, but it returns non-zero cost.

RKSimon added inline comments.Aug 24 2022, 5:48 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1993

A related question, I wonder if it's with a reason (or just unintended status quo) that getVectorInstrCost interface doesn't ask for a CostKind parameter?

This is on my TODO list that was waiting for the getInstructionCost refactor (D79483) - but I want to get some other CostKind changes (notably D132216) out of the way first.

mingmingl marked 3 inline comments as done.
mingmingl edited the summary of this revision. (Show Details)

Add a private method AArch64TTIImpl::getVectorInstrCostHelper as a helper function. This way, instruction-based method could share the core logic (e.g.,
returning zero cost if type is legalized to scalar).

Uploaded with full context and updated all affected test cases.

mingmingl added a comment.EditedAug 25 2022, 12:27 AM

I guess fhahn@ meant that for all useful scenarios, the instruction needs to have some uses (otherwise they are dead). For this case, the check should probably be tightened : instead of checking if the instruction has uses, it needs to examine the uses and differentiate integer uses that requires a fmov and other use contexts (which makes the extract noop).

Added a FIXME to estimate extract/insert cost with use-def contexts.

Checking use emptiness can pessimize cases where the cost is zero, but it returns non-zero cost.

Just a general comment: please try to include the full context when uploading a diff: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Yeah I will remember to do this from now on (already did it earlier this week upon suggestions from another reviewer, and a partial diff was purely unintentional.) Also will hold on updating the patch to keep the patch context for discussion (leave all comments open).

Uploaded with full context this time.

I'd like to point out the current patch still lacks test coverage for "virtual" vector instructions in print<cost-model> pass, due to the fact that instructions are physical in an IR. Will upload to get preliminary feedback on the overall direction -> it's feasible to get back test coverage, by calling op-based (not instruction based) getVectorInstrCost in print<cost-model> pass (the callsite of op-based cost API could be gated by a boolean option and turned on where it's needed).

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1887

With the refactoring this variable is gone. Updated patch should use UpperCase variables already.

1993

Thanks for making the cost-model changes! (mark this as done).

1993

Resolved by a refactoring that includes code sharing, comment update and dropping the uninteresting case.

Redundancy elimination might be an alternative option but I haven't got a chance to look into the alternative option yet, with the thought that fixing cost model is overall a good change as well :)*

Just to provide an update on this, looked into how to optimize by redundancy elimination this week -> machine-cse does eliminate the redundant machine instructions (and LICM/AArch64/extract-element.ll is supposed to be a reduced test case).

mingmingl retitled this revision from [AArch64][CostModel] Detects that ExtractElement at index is not free in AArch64 when result is used as integer. to [AArch64][CostModel] Detects that ExtractElement / InsertElement are not free in AArch64 with integer types..Aug 25 2022, 12:29 AM
mingmingl edited the summary of this revision. (Show Details)
davidxl added inline comments.Aug 25 2022, 9:23 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
68

Maybe changing 'VirtualInst' to be name 'hasNoRealUse' or flip it to 'hasRealUse'?

mingmingl marked an inline comment as done.
mingmingl retitled this revision from [AArch64][CostModel] Detects that ExtractElement / InsertElement are not free in AArch64 with integer types. to [AArch64][CostModel] Detects that {Extract,Insert}Element at lane 0 have the same cost as other lanes for real instructions that operates on integer types .
mingmingl edited the summary of this revision. (Show Details)

address comment.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h
68

Change to 'HasRealUse'. An instruction is a derived class of 'User', but I think it's fine to call it a use without ambiguity here.

The patch looks reasonable to me. Check if @fhahn @RKSimon have further comments.

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1855–1877

VirtualInst--> HasRealUse.

mingmingl updated this revision to Diff 458884.Sep 8 2022, 2:45 PM
mingmingl marked an inline comment as done.

fix comment after renaming variable (VirtualInst->HasRealUse).

RKSimon added inline comments.Sep 9 2022, 8:59 AM
llvm/test/Analysis/CostModel/AArch64/kryo-inseltpoison.ll
9 ↗(On Diff #458884)

update the comment

llvm/test/Analysis/CostModel/AArch64/kryo.ll
9

update the comment

mingmingl updated this revision to Diff 459097.Sep 9 2022, 9:39 AM
mingmingl marked 2 inline comments as done.

updated test comments

RKSimon accepted this revision.Sep 9 2022, 9:42 AM
This revision is now accepted and ready to land.Sep 9 2022, 9:42 AM

Thanks for reviews! Going to commit.

This revision was landed with ongoing or failed builds.Sep 9 2022, 10:05 AM
This revision was automatically updated to reflect the committed changes.