This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Future] Add Support For Functions That Do Not Use A TOC.
ClosedPublic

Authored by stefanp on Jan 29 2020, 1:54 PM.

Details

Summary

On PowerPC most functions require a valid TOC pointer.

This is the case because either the function itself needs to use this pointer to access the TOC or because other functions that are called from that function expect a valid TOC pointer in the register R2. The main exception to this is leaf functions that do not access the TOC since they are guaranteed not to need a valid TOC pointer.

This patch introduces a feature that will allow more functions to not require a valid TOC pointer in R2.

The feature works as follows:

  • If a function requires a TOC pointer because it accesses the TOC then that will remain as it was before.
  • If a funcction requires a TOC pointer only because it has a call to another function that may require a TOC pointer then we can modify the call site as follows:

Call Site Before

bl callee
nop

Call Site After

bl callee@notoc

The @notoc relocation tells the linker that the caller does not have a
valid TOC pointer and the nop after the call is removed because it is no
longer needed as we do not need to restore R2 after the call.

This work is done as preparation for upcoming PCRelative work to be done under the future CPU.
The support will only be added under future CPU for now.

Diff Detail

Event Timeline

stefanp created this revision.Jan 29 2020, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 1:54 PM
jsji added a reviewer: Restricted Project.Jan 29 2020, 2:09 PM
jsji added a project: Restricted Project.
nemanjai added inline comments.Jan 30 2020, 6:31 AM
llvm/include/llvm/BinaryFormat/ELF.h
395–396

This ternary operator nesting is so deep that it is now unreadable. Please refactor it. Also, what is supposed to happen if Offset is 1, 2 or 3? Previously, we set the return value to zero, but now you are just returning that value (shifted). Is that what you want?

Perhaps something like (using Log2 from MathExtras.h):

unsigned Val = 0;
if (Offset < 4)
  Val = Offset;
else
  Val = Log2(Offset);
Val = Val >= 6 ? 6 : Val;
Val = Val < 0 ? 0 : Val;
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
507

It is not clear how these additions are related to what this patch does.

564

What does the name signify? Is the symbol for the callee function TOC Pointer relative? I would imagine it is not.

1524

I don't really understand the need for this. If the function has calls, presumably R2 is marked as used. On the other hand, if it only has @notoc calls, it shouldn't need to set up R2. The latter is what you are adding here and the former is already handled. So under what conditions do we need global and local entry points in functions that have calls but do not mark R2 as used?

1733

The formatting looks weird. It may be right, just looks weird. In either case, it is easy to fix this ambiguity if you have your local changes committed and run:
git clang-format <previous hash>
(it will clang-format all the changes since the commit with the hash you specified).

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
872

I don't think this is the right place for this code. Please note that the enclosing function will only run if skipFunction(MF.getFunction()) is false. Since this is required for correctness rather than optimization, it should be added in a place where it cannot be skipped.

sfertile added inline comments.
llvm/include/llvm/BinaryFormat/ELF.h
395–396

Why not move this to PPCMCTargetDesc.cpp since thats the only place that calls it in the monorepo.

395–396

There are only certain values that make sense: The 'Symbol Values' section of the ABI describes the valid values. For example 1, 2 or 3 would be nonsense. The offset from the GEP to the LEP is 0 if we don't need to setup a toc-pointer, and at least 4-bytes if we do need to setup a toc-pointer.

+1 to Nemanjas suggestion of using Log2 to calculate the encoding. I would add a bit more error handling though:

  • Assert the offset is within the expected range of [0, 64]
  • Early return for an Offset of 0.
  • Assert the offset is a power of 2.
  • return the log of the value.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
184

Can Res be evaluated to 1? It has a very special meaning: The local and global entry points are the same, and r2 should be treated as caller-saved for callers. I'm not aware of LLVM having support for that yet but I could be mistaken. If it does already have support do we have a lit test for it?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5120

Minor nit: I would add a distinct conditional for this before the above comment:

if (Subtarget.isUsingPCRelativeCalls()) {
  assert(Subtarget.is64BitELFABI() ...)
  return PPCISD::CALL_NOTOC
}
5423–5424

Minor nit: if ((Subtarget.is64BitELFABI() && !Subtarget.isUsingPCRelativeCalls()) || Subtarget.isAIXABI())

stefanp marked 9 inline comments as done.Feb 4 2020, 5:58 PM

Address a number of review comments.

llvm/include/llvm/BinaryFormat/ELF.h
395–396

I turned this into a switch statement to clean it up a little. Basically, the valid values are (0, 1, 4, 8, 16, 32, 64). We do now support a value of 1 with this patch. The new version of the function will report an error if the input Offset is not one of the acceptable values.
The reason I wanted to use a switch is because it makes which values are valid quite obvious.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
184

Yes, we can now have Res equal to 1. We did not have support for this before but we need support for this for the implementation of PCRelative. If a function has no uses of R2 but does have function calls with the @notoc relocation we need to use an st_other=1 for it.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
507

You are correct. These changes are not needed. I'm going to remove them.

564

Copy paste mistake on my part. I've changed the name.

1524

It used to be the case that R2 was marked as used on all function calls but that is no longer true at this point. This patch removes the marking of R2 as used on calls. (See the changes to PPCISelLowering). We have now removed the assumption that the caller needs to have a valid TOC pointer.

This else if will be hit in situations where a function does not need a valid TOC pointer but that function has calls to other, possibly external, functions. Basically, we only have @notoc calls. Since we do not know what the functions we are calling do with R2 we must assume that they clobber the register. Therefore, callers of our function need to know that this function does not preserve R2. Or, more accurately, we cannot guarantee that this function preserves R2. Therefore we need to emit a .localentry for this function such that the st_other is set to 1. (Like MCConstantExpr::create(1, OutContext)). This value for st_other still does not set up a TOC pointer and is just used by the linker to know which functions may clobber R2. The global and local entry points remain the same.

I'm going to add a comment in the code to try to explain this as best I can.

1733

Done.
Strangely, this line was not changed by clang-format but others were.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5120

Done.

5423–5424

Done.

stefanp updated this revision to Diff 242484.Feb 4 2020, 6:06 PM

Address review comments.

sfertile added inline comments.Feb 5 2020, 9:13 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
184

Yes, we can now have Res equal to 1. We did not have support for this before but we need support for this for the implementation of PCRelative.

My question wasn't "do we need to support an encoding of 1". I understand we do now need to support this. My question was can Res be evaluated to '1' which I think the answer here is no. I'll provide my reasoning why I think so and you can correct me if I am wrong.

We evaluate 'Res' through a generic interface in MCExpr. The MCExpr we build for the local offset is here. As you can see it is a subtract of the global entry label from the local entry label. I haven't went into MCExpr::evaluateAsAbsolute to verify but I doubt it has the context to determine if this subtract expression should be replaced by '1' instead of naively evaluating the subtraction.

A further complication is that we don't call emitLocalEntry if we don't use X2. I don't know what our intended behavior would be if we have a function that doesn't use X2 but has multiple @notoc calls. If we should encode '1' in ST_OTHER in that case, then we need to do so elsewhere.

stefanp marked an inline comment as done.Feb 10 2020, 10:24 AM
stefanp added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
184

I did discuss this with Sean offline but I want to add more information here for everyone else:
An st_other value of 1 is a special value and does not behave like the others. Normally an assembly file will have
.localentry @symbol, .LEP-.GEP where LEP is the local entry point and GEP is the global entry point. The linker can then compute that difference and assign st_other accordingly. However, for the case of 1 there is a problem because we will never have the local entry and the global entry one byte apart. That was the issue that Sean was talking about.
However, this patch also adds code to PPCLinuxAsmPrinter::EmitFunctionBodyStart where we introduce the assembly
.localentry @symbol, 1 for just this situation.

efriedma added inline comments.
llvm/test/MC/PowerPC/ppc64-localentry-error1.s
10

Only loosely related to this patch, but using report_fatal_error isn't user-friendly. We should print a proper parse error using reportError or something like that, if a user writes .localentry foo, 33

stefanp updated this revision to Diff 245209.Feb 18 2020, 10:46 AM

Addressed review comments as well as the issue with MO_ExternalSymbol. I've added a test case for the situation where MO_ExternalSymbol is required.

stefanp updated this revision to Diff 245470.Feb 19 2020, 10:55 AM

Fixup for tests test/MC/PowerPC/ppc64-localentry-error1.s and test/MC/PowerPC/ppc64-localentry-error2.s.

The changes that are needed are fairly minor but I would like to take another look. I promise to get to it immediately when you have the updated patch posted.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
242

I believe the cases can be rewritten more concisely as:

case 0:
case 1:
case 4:
case 8:
case 16:
case 32:
case 64:
  return Log2(Offset) << ELF::STO_PPC64_LOCAL_BIT;

That way we still only allow the expected values and we have a nice simple expression for what we return.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
846

Don't we also need to make sure that the caller does not have any uses of the TOC? I believe this assumes no uses of the TOC if we have PC-Rel enabled and are on the right object mode/ABI. But aren't there still possible uses of the TOC in the function (at least for now)?

852

s/Extrnal/External

Also, is this a temporary thing? IIRC, the reason we can't tail call external functions is because they may clobber the TOC. But if we no longer promise to maintain the TOC in this function (i.e. through the st_other bit), can we not actually do the tail call opt?
Please note that I am not suggesting changing this in this patch, just that we may want to add a FIXME explaining that this is temporary.

1528

Thanks for the detailed explanation here. I would just amend this slightly:

1) A leaf function that does not use R2 (or treats it as
   callee-saved and preserves it).
llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
135

Please add the following comment:

// FIXME: This introduces another complete traversal of the instructions
// in the function in the common case (function is not skipped). Although
// this is less than ideal for compile time, this code will go away once
// our PC-Rel implementation is complete.
llvm/test/MC/PowerPC/ppc64-localentry-error1.s
10

@efriedma Has this been adequately addressed above? The message (and termination without producing an object file) replicates the behaviour of the system assembler.

anil9 added a subscriber: anil9.Mar 3 2020, 1:41 PM
anil9 added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
241

Nit : The message is fine just that we are allowing 0 which is not a power of 2, and we are not allowing 2 which is a power of 2. In the second case we will be seeing this message. It is tempting to bring up the relation of power of 2 to this code, but actually it does not exist so it is better not to talk about power of 2 here. This got me confused too in the beginning.

242

Lo2 wont work here, Log2 of 0 will be negative infinity and of 1 will be zero.

nemanjai added inline comments.Mar 3 2020, 7:39 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
241

Please note the comment elsewhere - the message is the message the GNU assembler produces for the same case.

242

Ah, of course. Good point. It would have to be:

case 0: return 0;
case 1:
case 4:
case 8:
case 16:
case 32:
case 64:
  return Log2(Offset) << ELF::STO_PPC64_LOCAL_BIT;
anil9 added inline comments.Mar 4 2020, 6:24 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
104

Similar to the changes you made to this file, should you also be changing the function "shouldForceRelocation"

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
88–89

Was wondering if you should be adding the case "case PPC::fixup_ppc_br24_notoc:" here or not.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
246

If you really need to use log2, you can separate out the case of 0,1 and then use log2 or 4,8 ..... 64.
But yes the error message still has to specify more information since 2 & 128 and above are not allowed and also 2^-(inf) = 0 is allowed.

nemanjai added inline comments.Mar 5 2020, 5:01 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp
246

Right, yes the two cases should be handled separately.

I definitely do not agree about the error message. There is no compelling reason to diverge from system tools for things like this. Furthermore, there is absolutely no issue with not accepting some powers of two. The message after all says that the expression is not a valid power of 2, not that the expression is not a power of 2.

stefanp marked 5 inline comments as done.Mar 5 2020, 2:37 PM
stefanp added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
104

That's a good point. I'll add it.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
88–89

I will add it up here and remove the case below.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
846

At this point all tail calls use the same TOC as the caller so it is safe to mark all of them as @notoc for now. When we expand tail calls to be able to call into other functions (as you mentioned below) we will have to add the guard.

852

Yes this should be a temporary thing. I will add a FIXME comment and I will also add that we need to add a guard to make sure that the caller does not use a TOC when we start implementing this addition.

1528

Done.

stefanp updated this revision to Diff 248612.Mar 5 2020, 2:38 PM

Address Review Comments

stefanp updated this revision to Diff 249205.Mar 9 2020, 1:49 PM

Disabled Tail Calls for when using PC Relative. That will be fixed in a later patch.

I really think we should add significantly more testing to this patch. We should have test cases to cover the following (we can also just add run lines with -mcpu=future to existing tests):

  • Calls from functions that use the TOC (i.e. globals, etc.)
  • Calls from functions that don't use the TOC
  • Indirect calls
  • Tail calls (i.e. that they are disabled)
  • Handling of X2 in leaf functions (since this has changed AFAICT)
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
507

A test case demonstrating the need for this as well as a code comment is needed here.

561

It seems kind of surprising that we do not need to do anything special here for the existing versions of BL8/BL8_NOP/BL8_TLS/... but we do need it for this. Can you explain why that is?

839

Since we are not doing anything in these cases, perhaps we should not add them in this patch but defer that until the patch that adds handling for them.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
5099

So we will currently use BCTRL + LD 2, 24(1) for indirect calls even with PC-Relative? I suspect that the plan is for that to change in an upcoming patch and for this to be moved up before the previous check. If that is so, please add a comment to that end.

llvm/lib/Target/PowerPC/PPCMIPeephole.cpp
279

I think we don't need to do anything in the if (!HasR2) case. Can we just use an early exit for that?

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
157

I initially started typing out a comment that I don't think this is safe. But then, looking through the code I convinced myself it is safe. However, I think a comment is in order because of the non-obviousness of the fact that this is safe.
Perhaps:

// We do not need to treat R2 as callee-saved when using PC-Relative calls
// because any direct uses of R2 will cause it to be reserved. If the function
// is a leaf or the only uses of R2 are implicit uses for calls, the calls
// will use the @notoc relocation which will cause this function to set the
// st_other bit to 1, thereby communicating to its caller that it arbitrarily
// clobbers the TOC.
llvm/lib/Target/PowerPC/PPCSubtarget.cpp
232

I believe we also need to add a check for ELFv2ABI here since I don't think we want to do this on other ABI's, right?

stefanp updated this revision to Diff 251776.Mar 20 2020, 3:06 PM

Addressed review comments.
Added new test cases to cover a number of different call situations.

After looking at Nemanja's comment (and trying to answer his questions) I did some digging and realized that I could simplify a part of the code significantly.

stefanp marked an inline comment as done.Mar 20 2020, 3:07 PM
stefanp added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
561

You are correct. I do not actually need this code here. I have changed the patch and I think it looks better now.

amyk added a subscriber: amyk.Mar 21 2020, 3:26 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCScheduleP9.td
43–44

nit: We should add PC relative mem ops to the list in the comment.

llvm/test/CodeGen/PowerPC/pcrel-call-linkage-leaf.ll
2

It would be good to add -ppc-asm-full-reg-names to this, and the rest of the test cases.

nemanjai accepted this revision.Mar 23 2020, 7:41 AM

LGTM. The remaining comments are just nits that you can address on the commit. Thanks for your patience through this review and for addressing all the comments.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1467

Nit: At first glance, I thought this should have been:
Subtarget->isELFv2ABI() && (UsesX2OrR2 || PPCFI->usesTOCBasePtr())

But I realize now that it is correct the way you wrote it. There should be a comment explaining it though. Perhaps something along the lines of:

- // Only do all that if the function uses r2 in the first place.
+ // Only do all that if the function uses R2 as the TOC pointer
+ // in the first place. We don't need the global entry point if the
+ // function uses R2 as an allocatable register.
This revision is now accepted and ready to land.Mar 23 2020, 7:41 AM
This revision was automatically updated to reflect the committed changes.

For what it's worth, this patch introduces a regression when building the Linux kernel's powernv_defconfig with Clang and linking with ld.bfd (for some reason, ld.lld is fine):

$ make -j$(nproc) -s AR=llvm-ar ARCH=powerpc CC=clang CROSS_COMPILE=powerpc64le-linux-gnu- NM=llvm-nm O=out/ppc64le OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump OBJSIZE=llvm-size READELF=llvm-readelf STRIP=llvm-strip distclean powernv_defconfig zImage.epapr
...
arch/powerpc/kernel/head_64.o: in function `ppc64_runlatch_on_trampoline':
(.text+0x2b00): call to `__ppc64_runlatch_on' lacks nop, can't restore toc; (toc save/adjust stub)
powerpc64le-linux-gnu-ld: final link failed: bad value
...

$ make -j$(nproc) -s AR=llvm-ar ARCH=powerpc CC=clang CROSS_COMPILE=powerpc64le-linux-gnu- LD=ld.lld NM=llvm-nm O=out/ppc64le OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump OBJSIZE=llvm-size READELF=llvm-readelf STRIP=llvm-strip distclean powernv_defconfig zImage.epapr; echo ${?}
...
0

$ clang --version | head -n1
ClangBuiltLinux clang version 11.0.0 (git://github.com/llvm/llvm-project 6c4b40def77622a5cf62a219ef4af63dc876e144)

$ powerpc64le-linux-gnu-ld --version | head -n1
GNU ld (GNU Binutils) 2.34

$ git show -s --format=short | cat
commit 5364abc57993b3bf60c41923cb98a8f1a594e749
Merge: 854e80bcfdafb f09d3174f002e
Author: Linus Torvalds <torvalds@linux-foundation.org>

    Merge tag 'arc-5.7-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/vgupta/arc

Relevant source files I believe:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/process.c?h=5364abc57993b3bf60c41923cb98a8f1a594e749#n2106

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/exceptions-64s.S?h=5364abc57993b3bf60c41923cb98a8f1a594e749#n2182

My PowerPC knowledge is non-existent so I do not know if this is expected behavior but I would appreciate some input. Cheers!

For what it's worth, this patch introduces a regression when building the Linux kernel's powernv_defconfig with Clang and linking with ld.bfd (for some reason, ld.lld is fine):

Changing code generation for subtargets that do not explicitly include PC-Relative support was not intended. Thank you for reporting this. Fix is coming soon.

For what it's worth, this patch introduces a regression when building the Linux kernel's powernv_defconfig with Clang and linking with ld.bfd (for some reason, ld.lld is fine):

Changing code generation for subtargets that do not explicitly include PC-Relative support was not intended. Thank you for reporting this. Fix is coming soon.

Should be fixed in 5b18b6e9a84d985c0a907009fb71de7c1943bc88. @nathanchance can you please confirm that the kernel build is back to normal? Both Stefan and I were able to build it according to your instructions after that commit.

@nemanjai thank you for that fix! Unfortunately, I am still seeing some errors with the modules target:

$ make -j$(nproc) -s AR=llvm-ar ARCH=powerpc CC=clang CROSS_COMPILE=powerpc64le-linux-gnu- NM=llvm-nm O=out/ppc64le OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump OBJSIZE=llvm-size READELF=llvm-readelf STRIP=llvm-strip distclean powernv_defconfig modules
...
/tmp/int4-4420da.s: Assembler messages:
/tmp/int4-4420da.s:176: Error: can't resolve `.TOC.' {*UND* section} - `.Lfunc_gep1' {*UND* section}
clang-11: error: assembler command failed with exit code 1 (use -v to see invocation)
...

Sorry I did not catch this initially, we were just building the kernel image before, I did not realize that there was going to be a separate set of issues with modules :/ our CI only builds the kernel image due to time restrictions but I build everything locally, hence how I did not catch that sooner.

The command above is just for modules for easy triage. You can also just build one translation unit like lib/raid6/int4.o, that one errors for me. I build everything locally with the all target.

@nemanjai thank you for that fix! Unfortunately, I am still seeing some errors with the modules target:

$ make -j$(nproc) -s AR=llvm-ar ARCH=powerpc CC=clang CROSS_COMPILE=powerpc64le-linux-gnu- NM=llvm-nm O=out/ppc64le OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump OBJSIZE=llvm-size READELF=llvm-readelf STRIP=llvm-strip distclean powernv_defconfig modules
...
/tmp/int4-4420da.s: Assembler messages:
/tmp/int4-4420da.s:176: Error: can't resolve `.TOC.' {*UND* section} - `.Lfunc_gep1' {*UND* section}
clang-11: error: assembler command failed with exit code 1 (use -v to see invocation)
...

Sorry I did not catch this initially, we were just building the kernel image before, I did not realize that there was going to be a separate set of issues with modules :/ our CI only builds the kernel image due to time restrictions but I build everything locally, hence how I did not catch that sooner.

The command above is just for modules for easy triage. You can also just build one translation unit like lib/raid6/int4.o, that one errors for me. I build everything locally with the all target.

This is also a regression after this patch or something that was broken before?

This is also a regression after this patch or something that was broken before?

Yes. If I checkout to f3bf25eb663c49e821120fd311677764d2bad79e, modules builds successfully but with 6c4b40def77622a5cf62a219ef4af63dc876e144, I get that error, with or without 5b18b6e9a84d985c0a907009fb71de7c1943bc88.

This is also a regression after this patch or something that was broken before?

Yes. If I checkout to f3bf25eb663c49e821120fd311677764d2bad79e, modules builds successfully but with 6c4b40def77622a5cf62a219ef4af63dc876e144, I get that error, with or without 5b18b6e9a84d985c0a907009fb71de7c1943bc88.

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

This is also a regression after this patch or something that was broken before?

Yes. If I checkout to f3bf25eb663c49e821120fd311677764d2bad79e, modules builds successfully but with 6c4b40def77622a5cf62a219ef4af63dc876e144, I get that error, with or without 5b18b6e9a84d985c0a907009fb71de7c1943bc88.

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

Correct!

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

Correct!

Hi Nathan, I pushed 04eae396176ccebe359bbb47ca1458a6377bbc4f after building all successfully. When you get a chance, could you please confirm that it is back to normal? Thank you for your patience and for reporting this.

Awesome. Thank you. I'll work on getting that fixed. To verify everything, I just replace modules in the invocation with all?

Correct!

Hi Nathan, I pushed 04eae396176ccebe359bbb47ca1458a6377bbc4f after building all successfully. When you get a chance, could you please confirm that it is back to normal? Thank you for your patience and for reporting this.

Yes, everything appears good to me. I will let you all know if anything else comes up in my other tests. Thank you very much for the quick patches!