Another minor tweak to clang/test/CodeGen/PowerPC/ppc64le-varargs-f128.c.

Retry, wrong patch uploaded last time.

Minor fix to the clang/CodeGen/ppc64le-varargs-f128.c test.

Removed the empty string in the getName() invocation for the outlined helper's name.

I had to cut context lines to 2 to fit in the 8MB limit. It looks like there are a few files that absorb way more line diff than makes sense, but I haven't investigated way. One particular offender is clang/test/OpenMP/nvptx_SPMD_codegen.cpp, which loses 23KLOC. I think some of the NVPTX files are not idempotent under update_cc_test_checks in master, but I haven't checked again to make sure.

Update relevant test outputs.

add comments

add comments

Add comment

Refined logic so that stackrestores of the same stacksave do
not inhibit removal in the same instcombine iteration.

Thanks; Could you commit this on my behalf?

Improved the test to include multiple stackrestores.

Improved comments

Certainly, done

split from NFC refactor

That feature already exists - use a plain old function declaration :)

I don't know much about the ELF format... but this works today? We can define a resolver in a different TU and it WORKS thanks to the linker? So there is perhaps something?

The ifunc symbol that is emitted in the TU with the undefined resolver loses its connection to the resolver and the calls to the ifunc are instead bound against the resolver itself (which is absolutely not what you want).

It sort-of-works only because you define the ifunc in both translation units (with the same name). But looks like it behaves incorrectly for references to the ifunc in the translation unit where the resolver is only declared, not defined:

> cat example1.ll
@single_version_ifunc = weak_odr dso_local ifunc void (), void ()* ()* @single_version_resolver
define void ()* @single_version_resolver() {
  ret void ()* null
}

I'm referring you again to the start of my explanation (the first three paragraphs); the object file format (ELF) literally cannot express the semantics you're asking for. You're asking for it to support a symbol in a special kind of undefined state:
ABC = "undefined, but becomes defined to the value of a different symbol XYZ if it ever becomes defined".

Added comment about call-with-side-effects, added same-BB-check

I have no commit access, might I ask that you commit this on my behalf?

My only lead for this change was that I'd assumed that at some point this code did not emit the aforementioned warning: https://github.com/llvm/llvm-project/blob/811b1736d91b301f59fbaa148ff55c32f90a3947/compiler-rt/cmake/Modules/CompilerRTUtils.cmake#L346-L352

ping :)

That's already what happens, both before and after this change, because both invocations (add_llvm_library before the change, and add_llvm_component_library after the change) pass the BUILDTREE_ONLY flag.
Therefore the library doesn't get installed either way.

I think that an llvm 'component library' is a logical library that llvm exposes outwards.
It becomes query-able from llvm-config, and it declares other components it depends on for internal dependency management, which is then exposed outwards also.
For example, when I invoke llvm-config --ldflags --libs testingsupport, after the fix, I get this output:

-L/path/to/build/dir/lib
-lLLVMTestingSupport -lLLVMSupport -lLLVMDemangle

Added Tom (https://reviews.llvm.org/D70179, creation of add_llvm_component_library), and Michał (https://reviews.llvm.org/D55891, includes introduction of warning I'm trying to tackle)

ping :)

ping :)

Alright, yes, that makes a lot of sense to me. The patch already uses ZExt, so I think it could be considered ready.

Changed to use ZExtOrTrunc. Note change in X86/shift_minsize.ll

Modified to use DAG.getLibInfo().getIntSize() as @bjope suggested in llvm-dev.

True, at SelectionDAGBuilder time that invariant holds. So just speedbump removal / potential copypasta bug prevention? 😛

Added fshr.i128 test, refreshed another small diff in AArch64

Rebased on top of main.

Fixed clang-format.
Updated X86 and AArch64 shift_minsize test output, and refreshed labels for RISCV shifts test output.

Was sure I checked upstream, but I didn't.

I cannot commit the patch myself because I don't have commit access, might I trouble you with that? :)

Updated the test per @MaskRay 's suggestion.

I have updated the diff to include the changes you suggested. However, as you said, for the test to have a chance of passing, D82745 is required. I will send out a mail to discuss the design issue on llvm-dev soon.

The problem with what @DmitryPolukhin suggested (which he later acknowledged in an additional comment), is that getBaseObject() returns a GlobalObject, but GlobalIFunc does not inherit from GlobalObject, so that can't compile.

@tejohnson would simply not emitting an AliasSummary in computeAliasSummary when getBaseObject() returns nullptr be sufficient, then?

In D81911#2137237, @DmitryPolukhin wrote:

I did a bit of archeology and it turns out that getBaseObejct was part of moved from GlobalAlias to GlobalIndirectSymbol in https://github.com/llvm/llvm-project/commit/95549497ec8b5269f0439f12859537b7371b7c90
It looks like the simplest solution is to handle nullptr from getBaseObejct in computeAliasSummary...

How do we want to proceed, then? Does my analysis make sense?

As far as I can tell, GlobalIndirectSymbol is there to share code between GlobalAlias and GlobalIFunc.
This shared code is mostly a common getBaseObject() implementation, which just delegates to getIndirectSymbol() (== getOperand(0)).
For GlobalAliases, the indirect symbol is the aliasee. For GlobalIFuncs, the indirect symbol is the resolver function.

Doesn't that mean that GlobalIFunc shouldn't really inherit from GlobalIndirectSymbol (and then GlobalIndirectSymbol loses the justification for its existence)?
I get the feeling like there's an expectation for getBaseObject to be idempotent (since it pierces through ConstantExprs and GlobalAliases), but GlobalIndirectSymbol::getBaseObject takes the resolver (getOperand(0)) and performs the lookup on it.

Used the exact same fix as a local workaround until further counsel on the bug report, so looks good to me.
Thank you!

I added a test case that exercises the crash before the fix.
However, your comment made me realize that there are two separate issues + one question here:

1. getBaseObject() is currently unable to indirect through GlobalIFuncs (as the current fix tries to address).
2. computeAliasSummary() assumes that getBaseObject() can never return nullptr, while it has potential flows that return nullptr. In Verifier::visitGlobalAlias() I see that aliases-to-ConstantExpr-s are valid, but in Verifier::visitAliaseeSubExpr I see that alias cycles are invalid. If we assume that the module for which we generate a summary is valid, this question amounts to whether it is valid for the ConstantExpr case of an alias to ever return nullptr there. I can add an assert(Aliasee) / fatal(..) in computeAliasSummary.
3. Is it valid for a GlobalIFunc to have an alias as its resolver function operand? It sounds contrived, but valid nonetheless (e.g. if the resolver is in a different translation unit and they end up getting merged in LTO?). A GlobalIFunc with a resolver that is itself a GlobalIFunc sounds like an invalid construction to me, though. I don't see references to GlobalIFunc in Verifier.cpp to use as guiding principles for what would be considered valid. So, it sounds to me that with respect to module validity, in an alias-...-alias-ifunc-alias-...-resolverfunc chain there should only ever be a single ifunc at most.

Wondering what the most appropriate place for the test case is.
I can hit the crash by passing --module-summary to opt, but the crash is a bit removed from the underlying issue itself, I think.

Alright, looks like it's settled and ready then :)
I cannot commit the patch myself because I don't have commit access, so might I trouble one of you with that?

Updated title and description.
Added description for test, created PR in bugzilla, corrected PR reference to point at it.

Changed the fix to push the ThreadLocalMode copying from derived class GlobalVariable down to base class GlobalValue.
Added an appropriate test under llvm/test/Linker.