[AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets #71485

amy-kwan · 2023-11-07T04:29:27Z

This patch utilizes the -maix-small-local-exec-tls option to produce a faster,
non-TOC-based access sequence for the local-exec TLS model. Specifically, for
when the offsets from the TLS variable are non-zero.

In particular, this patch produces either a single:

addi/la with a displacement off of R13 plus a non-zero offset for when an address is calculated, or
load or store off of R13 plus a non-zero offset for when an address is calculated and used for further
access where R13 is the thread pointer, respectively.

In order to produce a single addi or load/store off of the thread pointer with a non-zero offset,
this patch also adds the necessary support in the assembly printer when printing these instructions.

Specifically:

The non-zero offset is added to the TLS variable address when the address of the
TLS variable + it's offset is less than 32KB.
Otherwise, when the address of the TLS variable + its offset is greater than 32KB, the
non-zero offset (and a multiple of 64KB) is subtracted from the TLS address.

This handling in the assembly printer is necessary to ensure that the TLS address + the non-zero offset
is between [-32768, 32768), so that the total displacement can fit within the addi/load/store instructions.

This patch is meant to be a follow-up to 3f46e54 (where the
optimization occurs for when the offset is zero).

github-actions · 2023-11-07T04:31:19Z

✅ With the latest revision this PR passed the C/C++ code formatter.

stephenpeckham

The testcase updates look reasonable, but I didn't review every change.

stephenpeckham · 2023-11-08T22:06:18Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

@@ -1514,13 +1522,41 @@ void PPCAsmPrinter::emitInstruction(const MachineInstr *MI) {
  case PPC::LWA: {
    // Verify alignment is legal, so we don't create relocations
    // that can't be supported.
-    unsigned OpNum = (MI->getOpcode() == PPC::STD) ? 2 : 1;
+    unsigned OpNum;
+    if (Subtarget->hasAIXSmallLocalExecTLS())


Why does hasAIXSmallLocalExecTLS() affect OpNum? Is a comment needed?

stephenpeckham · 2023-11-08T22:09:07Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+        int64_t Offset = MO.getOffset();
+        // Non-zero offsets for lwa/ld/std require special handling and are
+        // handled here.
+        if (!Offset)


The test is opposite the preceding comment, and I wouldn't use a 'break' inside 2 if statments. Why not use if (Offset) { ...

stephenpeckham · 2023-11-08T22:09:24Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+          break;
+
+        LowerPPCMachineInstrToMCInst(MI, TmpInst, *this);
+        if (Offset) {


Is this check redundant?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

stephenpeckham · 2023-11-08T22:18:20Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+      int64_t Offset = MO.getOffset();
+      // Non-zero offsets for loads/stores require special handling and are
+      // handled here. For `addi`, all offsets are handled here.
+      if (!Offset && !IsMIADDI8)


Same comment as above: I would use if (Offset || IsMIADDI8) {...

stephenpeckham · 2023-11-08T22:19:00Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+// we need to create a new MCExpr that adds the non-zero offset to the address
+// of the local-exec variable that will be used in either an addi, load or
+// store. However, the final displacement for these instructions must be
+// between [-32768, 32768), so if the TLS address + it's non-zero offset is


Suggested change

// between [-32768, 32768), so if the TLS address + it's non-zero offset is

// between [-32768, 32768), so if the TLS address + its non-zero offset is

stephenpeckham · 2023-11-09T16:26:51Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+    // Handle the written offset for cases where:
+    //   address of the TLS variable + the offset is greater than 32KB.
+
+    // Get the address in the range of 0 to 64KB.


I think you can simplify this.The amount you have to subtract is ((FinalAddress + 32768 ) & ~0xFFFF)

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

amy-kwan · 2023-11-16T06:15:37Z

llvm/test/CodeGen/PowerPC/aix-small-local-exec-tls-largeaccess.ll

-; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    la r3, mySmallLocalExecTLS5[TL]@le(r13)
-; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r4, 332(r3)
+; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r4, mySmallLocalExecTLSv1[TL]@le+24(r13)
+; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r3, mySmallLocalExecTLS2[TL]@le-65216(r13)


Currently, the assembly prints:

TLSVar[storageMappingClass]@le + OffsetDelta(r13)

Where OffsetDelta = Offset - (Multiple of 64KB). Basically, it will print the single OffsetDelta value.

Stephen and I discussed this, and perhaps another way we could print it is the more explicit way of separating everything out:

TLSVar[storageMappingClass]@le + Offset - (Multiple of 64KB) (r13)

I experimented with this a bit and an example of the assembly would look like:

stw r3, (mySmallLocalExecTLS2[TL]@le+320)-65536(r13)

Not sure if that's clearer or looks better/worse, but the current way might look a bit cleaner.

If anyone has any opinions on this, please let me know.

I think the alternate way would look be better, because it's easier to debug or understand the assembly code. In addition, the "-65536" is required because of an AIX assembler limitation. If the assembler were fixed, the extra subtraction could be eliminated. But I could accept either output format.

amy-kwan

Partial group code review comments.

Also:

~~Title should be updated.~~
~~Might be good to add the zero-based offset patch in the description.~~

Done the above two items.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

amy-kwan · 2023-11-22T20:06:24Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

+  // InitialADDI is the addi feeding into N (also an addi), and the addi that
+  // we want optimized out.
+  SDValue InitialADDI = N->getOperand(0);
+  if (!InitialADDI.isMachineOpcode() ||


Might be better to separate the checks into a separate function so we can check it in the load/store case, too.

amy-kwan · 2023-11-22T20:06:43Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

@@ -7735,7 +7792,15 @@ void PPCDAGToDAGISel::PeepholePPC64() {
        ImmOpnd = CurDAG->getTargetConstant(Offset, SDLoc(ImmOpnd),
                                            ImmOpnd.getValueType());
      } else if (Offset != 0) {
-        continue;
+        if (!HasAIXSmallLocalExecTLS)


Missing checks for thread pointer/TLS variable or not?
Double check this.

amy-kwan · 2023-11-22T20:12:47Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

@@ -7735,7 +7792,15 @@ void PPCDAGToDAGISel::PeepholePPC64() {
        ImmOpnd = CurDAG->getTargetConstant(Offset, SDLoc(ImmOpnd),
                                            ImmOpnd.getValueType());
      } else if (Offset != 0) {
-        continue;
+        if (!HasAIXSmallLocalExecTLS)


Might be better to do:

if (HasAIXSmallLocalExecTLS) { ... } else continue;

stephenpeckham

I think the latest changes look good

stephenpeckham · 2023-11-27T21:41:18Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

    const MachineOperand &MO = MI->getOperand(OpNum);
    if (MO.isGlobal()) {
      const DataLayout &DL = MO.getGlobal()->getParent()->getDataLayout();
      if (MO.getGlobal()->getPointerAlignment(DL) < 4)
        llvm_unreachable("Global must be word-aligned for LD, STD, LWA!");
+
+      // A faster non-TOC-based local-exec sequence is represented by
+      // `lwa`/`ld`/`std` directingly loading or storing off of the thread


A nit, but there are a lot of load and store instructions. I don't think you need to list lwa, ld, and std explicitly.

stephenpeckham · 2023-11-27T21:55:11Z

llvm/test/CodeGen/PowerPC/aix-small-local-exec-tls-largeaccess.ll

-; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    la r3, mySmallLocalExecTLS5[TL]@le(r13)
-; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r4, 332(r3)
+; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r4, mySmallLocalExecTLSv1[TL]@le+24(r13)
+; SMALL-LOCAL-EXEC-SMALLCM64-NEXT:    stw r3, mySmallLocalExecTLS2[TL]@le-65216(r13)


I think the alternate way would look be better, because it's easier to debug or understand the assembly code. In addition, the "-65536" is required because of an AIX assembler limitation. If the assembler were fixed, the extra subtraction could be eliminated. But I could accept either output format.

stephenpeckham · 2023-11-27T21:58:19Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+      // Such instructions do not otherwise arise.
+      if ((MO.getTargetFlags() & PPCII::MO_TPREL_FLAG) != 0) {
+        assert(HasAIXSmallLocalExecTLS &&
+               "lwa/ld/std with thread-pointer only expected with "


Another nit: I would change "lwa/ld/std" to "load/store"

syzaara · 2023-11-30T16:43:48Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

    const MachineOperand &MO = MI->getOperand(OpNum);
    if (MO.isGlobal()) {
      const DataLayout &DL = MO.getGlobal()->getParent()->getDataLayout();
      if (MO.getGlobal()->getPointerAlignment(DL) < 4)
        llvm_unreachable("Global must be word-aligned for LD, STD, LWA!");
+
+      // A faster non-TOC-based local-exec sequence is represented by
+      // directingly loading or storing off of the thread pointer and with


directingly -> directly

syzaara · 2023-11-30T16:45:00Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+  // non-zero offset to the TLS variable address.
+  // For when TLS variables are extern, this is safe to do because we can
+  // assume that the address of extern TLS variables are zero.
+  if ((FinalAddress < 32768) || IsGlobalADeclaration)


Maybe we don't need || IsGlobalADeclaration since in
ptrdiff_t FinalAddress = (TLSVarAddress + Offset);
TLSVarAddress was initialized to 0 for IsGlobalADeclaration.
So it will already pass FinalAddress < 32768 for IsGlobalADeclaration.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

amy-kwan · 2024-01-08T21:16:16Z

Ping.

stephenpeckham

Other than a couple of nits, I think the code and the generated assembly look good. Factoring out the +/- 65536 in the assemble code helps anyone reading the the assembly listing.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

stephenpeckham

LGTM

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

diggerlin

LGTM.

diggerlin · 2024-01-25T16:40:16Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

@@ -7578,6 +7668,10 @@ void PPCDAGToDAGISel::PeepholePPC64() {
    if (isVSXSwap(SDValue(N, 0)))
      reduceVSXSwap(N, CurDAG);

+    // This optimization is performed for non-TOC-based local-exec accesses.
+    if (HasAIXSmallLocalExecTLS)
+      foldADDIForLocalExecAccesses(N, CurDAG);


continue the loop earlier will save compiler compile time ,
anyway, you can keep your code. I don't have a strong preference.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

diggerlin

LGTM

ecnelises · 2024-01-31T08:36:14Z

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

+    assert((InstDisp < 32768) ||
+           (InstDisp >= -32768) &&
+               "Expecting the instruction displacement for local-exec TLS "
+               "variables to be between [-32768, 32768)!");


(not a review comment)

Per my understanding, allow peephole for non-zero offsets but without changes to AsmPrinter, we'll get assembler error like The displacement must be greater than or equal to -32768 and less than or equal to 32767 (no-integrated-as).

Since code here is to rewrite offsets exceeding upper limit (32768) into negative, will this assert be hit if the offset is even larger? (for example, twice the size as objects in aix-small-local-exec-tls-largeaccess.ll)

For larger variables, the assert will be not triggered. This is because in the initial patch that introduced this feature, I only restricted this non-TOC-based access sequence if the size of the TLS variable is less than 32751 within PPCISelLowering.cpp.

constexpr uint64_t AIXSmallTlsPolicySizeLimit = 32751; .... // With the -maix-small-local-exec-tls option, produce a faster access // sequence for local-exec TLS variables where the offset from the TLS // base is encoded as an immediate operand. // // We only utilize the faster local-exec access sequence when the TLS // variable has a size within the policy limit. We treat types that are // not sized or are empty as being over the policy size limit. if (HasAIXSmallLocalExecTLS && IsTLSLocalExecModel) { Type *GVType = GV->getValueType(); if (GVType->isSized() && !GVType->isEmptyTy() && GV->getParent()->getDataLayout().getTypeAllocSize(GVType) <= AIXSmallTlsPolicySizeLimit) return DAG.getNode(PPCISD::Lo, dl, PtrVT, VariableOffsetTGA, TLSReg); }

So in the case where the size is 8187, the test case for the large variable in aix-small-local-exec-tls-largeaccess.ll looks like:

stw r3, mySmallLocalExecTLSv1[TL]@le(r13)

If the size is increased past AIXSmallTlsPolicySizeLimit, it will load from the TOC instead and do the regular local-exec sequence, which is expected:

ld r4, L..C0(r2) # target-flags(ppc-tprel) @mySmallLocalExecTLSv1 li r3, 1 add r4, r13, r4 stw r3, 0(r4) . . .

Hope the above answers your question.

…sequence for non-zero offsets This patch utilizes the -maix-small-local-exec-tls option to produce a faster, non-TOC-based access sequence for the local-exec TLS model, specifically for when the offsets from the TLS variable are non-zero. In particular, this patch produces either a single: - addi/la with a displacement off of R13 plus a non-zero offset for when an address is calculated, or - load or store off of R13 plus a non-zero offset for when an address is calculated and used for further access Where R13 is the thread pointer, respectively. In order to produce a single addi or load/store off of the thread pointer with a non-zero offset, this patch also adds the necessary support in the assembly printer when printing these instructions. Specifically: - The non-zero offset is added to the TLS variable address when the address of the TLS variable + it's offset is less than 32KB. - Otherwise, when the address of the TLS variable + its offset is greater than 32KB, the non-zero offset (multiplied by a multiple of 64KB) is subtracted from the TLS address. This handling in the assembly printer is necessary to ensure that the TLS address + the non-zero offset is between [-32768, 32768), so that the total displacement can fit within the addi/load/store instructions.

…move unnecessary breaks

… for folding

…ry variables, etc.

ecnelises · 2024-02-01T03:32:01Z

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

+  // access. The first operand of InitialADDI should be the thread pointer,
+  // which has been checked in isEligibleToFoldADDIForLocalExecAccesses().
+  SDValue TPRegNode = InitialADDI.getOperand(0);
+  RegisterSDNode *TPReg = dyn_cast_or_null<RegisterSDNode>(TPRegNode.getNode());


dyn_cast_or_null means TPRegNode.getNode() is nullable, is it true?

That's a good point. I think I wanted to change this initially but had forgot. I don't expect these to be null, so I think I will probably just do dyn_cast().

… offsets (llvm#71485) This patch utilizes the -maix-small-local-exec-tls option to produce a faster, non-TOC-based access sequence for the local-exec TLS model. Specifically, for when the offsets from the TLS variable are non-zero. In particular, this patch produces either a single: - addi/la with a displacement off of R13 plus a non-zero offset for when an address is calculated, or - load or store off of R13 plus a non-zero offset for when an address is calculated and used for further access where R13 is the thread pointer, respectively. In order to produce a single addi or load/store off of the thread pointer with a non-zero offset, this patch also adds the necessary support in the assembly printer when printing these instructions. Specifically: - The non-zero offset is added to the TLS variable address when the address of the TLS variable + it's offset is less than 32KB. - Otherwise, when the address of the TLS variable + its offset is greater than 32KB, the non-zero offset (and a multiple of 64KB) is subtracted from the TLS address. This handling in the assembly printer is necessary to ensure that the TLS address + the non-zero offset is between [-32768, 32768), so that the total displacement can fit within the addi/load/store instructions. This patch is meant to be a follow-up to 3f46e54 (where the optimization occurs for when the offset is zero).

amy-kwan added backend:PowerPC platform:aix labels Nov 7, 2023

amy-kwan requested review from hubert-reinterpretcast, nemanjai, mandlebug, stefanp-ibm and stephenpeckham November 7, 2023 04:29

amy-kwan self-assigned this Nov 7, 2023

amy-kwan requested a review from diggerlin November 7, 2023 15:56

stephenpeckham reviewed Nov 9, 2023

View reviewed changes

amy-kwan commented Nov 16, 2023

View reviewed changes

amy-kwan requested a review from stephenpeckham November 16, 2023 06:17

amy-kwan commented Nov 22, 2023

View reviewed changes

amy-kwan changed the title ~~[AIX][TLS] Optimize the -maix-small-local-exec-tls local-exec access sequence for non-zero offsets~~ [AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets Nov 22, 2023

stephenpeckham reviewed Nov 27, 2023

View reviewed changes

syzaara reviewed Nov 30, 2023

View reviewed changes

amy-kwan force-pushed the amy-kwan/small-local-exec-tls-nonzero-offsets branch from 789629a to c6d882b Compare December 1, 2023 05:50

syzaara reviewed Dec 7, 2023

View reviewed changes

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp Show resolved Hide resolved

amy-kwan force-pushed the amy-kwan/small-local-exec-tls-nonzero-offsets branch from fdcfeef to cf3957f Compare December 7, 2023 21:35

amy-kwan requested review from syzaara and stephenpeckham December 8, 2023 06:00

amy-kwan force-pushed the amy-kwan/small-local-exec-tls-nonzero-offsets branch 2 times, most recently from ec5f16e to c9ea0b0 Compare January 2, 2024 19:19

stephenpeckham reviewed Jan 8, 2024

View reviewed changes

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp Outdated Show resolved Hide resolved

amy-kwan requested a review from stephenpeckham January 10, 2024 14:54

stephenpeckham approved these changes Jan 10, 2024

View reviewed changes

diggerlin reviewed Jan 12, 2024

View reviewed changes

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp Show resolved Hide resolved

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp Show resolved Hide resolved

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp Outdated Show resolved Hide resolved

diggerlin reviewed Jan 12, 2024

View reviewed changes

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp Outdated Show resolved Hide resolved

amy-kwan force-pushed the amy-kwan/small-local-exec-tls-nonzero-offsets branch from 2601089 to 3563b6a Compare January 24, 2024 21:06

diggerlin reviewed Jan 26, 2024

View reviewed changes

diggerlin self-requested a review January 27, 2024 16:00

diggerlin approved these changes Jan 27, 2024

View reviewed changes

ecnelises reviewed Jan 31, 2024

View reviewed changes

amy-kwan added 16 commits January 31, 2024 11:24

Apply formatting changes

b4e7632

Address review comments: fix comments, simplify offset generation, re…

594cf6a

…move unnecessary breaks

Update comments and combine conditions

b5bcb25

Remove unnecessary whitespace

c34760e

Address review comments by pulling out checks for if addi is eligible…

504c5e0

… for folding

Remove comment and add asserts

98f5fcf

Update comments and condition

f3f7ca9

Update target flags for TLSGD variable

46c5079

Print assembly in the tlsVar+Offset-Delta method instead

211663a

Update target flag checks

ed6834a

Add hyphens to variable-to-address comment/assert

e023aef

Address various comments: moving around variables, removing unnecessa…

7e5a882

…ry variables, etc.

Remove unnecessary subtarget variable.

58fef76

Add an assert and common up code from load/stores

079869e

Update assert to early exit

9acad76

ecnelises reviewed Feb 1, 2024

View reviewed changes

Update to use dyn_cast() calls

cf5df00

amy-kwan force-pushed the amy-kwan/small-local-exec-tls-nonzero-offsets branch from 859368e to cf5df00 Compare February 1, 2024 13:57

amy-kwan merged commit 2a50921 into llvm:main Feb 1, 2024
3 of 4 checks passed

diggerlin mentioned this pull request Mar 21, 2024

[AIX][TLS] Produce a faster local-exec access sequence for the "aix-small-tls" global variable attribute #83053

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets #71485

[AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets #71485

amy-kwan commented Nov 7, 2023 •

edited

github-actions bot commented Nov 7, 2023 •

edited

stephenpeckham left a comment

stephenpeckham Nov 8, 2023

stephenpeckham Nov 8, 2023

stephenpeckham Nov 8, 2023

stephenpeckham Nov 8, 2023

stephenpeckham Nov 8, 2023

stephenpeckham Nov 9, 2023

amy-kwan Nov 16, 2023

stephenpeckham Nov 27, 2023

amy-kwan left a comment •

edited

amy-kwan Nov 22, 2023

amy-kwan Nov 22, 2023

amy-kwan Nov 22, 2023

stephenpeckham left a comment

stephenpeckham Nov 27, 2023

stephenpeckham Nov 27, 2023

stephenpeckham Nov 27, 2023

syzaara Nov 30, 2023

syzaara Nov 30, 2023

amy-kwan commented Jan 8, 2024

stephenpeckham left a comment

stephenpeckham left a comment

diggerlin left a comment

diggerlin Jan 25, 2024

diggerlin left a comment

ecnelises Jan 31, 2024

amy-kwan Jan 31, 2024

ecnelises Feb 1, 2024

amy-kwan Feb 1, 2024

	// between [-32768, 32768), so if the TLS address + it's non-zero offset is
	// between [-32768, 32768), so if the TLS address + its non-zero offset is

[AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets #71485

[AIX][TLS] Optimize the small local-exec access sequence for non-zero offsets #71485

Conversation

amy-kwan commented Nov 7, 2023 • edited

github-actions bot commented Nov 7, 2023 • edited

stephenpeckham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenpeckham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan commented Jan 8, 2024

stephenpeckham left a comment

Choose a reason for hiding this comment

stephenpeckham left a comment

Choose a reason for hiding this comment

diggerlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diggerlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan commented Nov 7, 2023 •

edited

github-actions bot commented Nov 7, 2023 •

edited

amy-kwan left a comment •

edited