drop old pass manager test from callbr-critical-edge-splitting2.ll since it does not add signal

• use CHECK-NEXT, drop -passes=loop-reduce

In D94470#2499394, @jyknight wrote:

This patch also fixes the test case in D88438 (and D88438 also fixes this test case).

Does that other review fix this test case intrinsically -- in that this patch is definitely not necessary if that is committed? Or might it just be fixed by accident -- e.g. this particular test-case doesn't trigger the problem anymore, but something else might, without this patch?

This patch also fixes the test case in D88438 (and D88438 also fixes this test case). So two different ways of solving similar bugs.
(ie. either would fix https://github.com/ClangBuiltLinux/linux/issues/1161 and https://github.com/ClangBuiltLinux/linux/issues/1252). I don't feel strongly either way; maybe reviewers could take a look at both approaches?

• run the same invocations for both tests, remove unnecessary target_triple

In D88438#2305129, @void wrote:

In D88438#2305068, @efriedma wrote:

The concept of splitting a critical edge is still relatively new to me; any thoughts on *why* it would not be ok to split a critical branch of an indirect-like jump?

The problem has to do with blockaddress.

For normal branches, splitting an edge is usually straightforward: you just introduce a new block that branches to the original block, and then you rewrite the branch in the predecessor. But suppose you have a block with two indirectbr predecessors, and you want to split the edge. In that case, the "rewrite the branch" step doesn't work: you can't rewrite the address argument to the indirectbr. (Or at least, you can't without performing some invasive rewrite like indirectbr->switch lowering.)

• add comments and FileCheck to tests

• mv my test

• remove comment from test, simplify lambda

• rebase, fix check for only non-default destination edges as per Bill
• looks like this also fixes Fixes: https://github.com/ClangBuiltLinux/linux/issues/1252 / D94470
• Added test case from D94470.

Patch fixes the reported bug; added test cases produces a crash before patch is applied. LGTM with minor nit.

@nathanchance also pointed out I had https://reviews.llvm.org/D88438 to fix a similar issue (I forgot about that patch, I should try it with this test case, too).

Patch LGTM; just curious about the comment, since that's not what's occurring in the test AFAICT. Thanks for the patch!

ah! probably not, let me rebuild

In D94470#2493986, @void wrote:

In D94470#2493138, @nickdesaulniers wrote:

I guess the presence of the profile data makes it difficult to use bugpoint to reduce the test case more?

It looks like I was able to reduce it. PTAL.

I guess the presence of the profile data makes it difficult to use bugpoint to reduce the test case more?

Sure. I'll commit it.

This differs from the little endian targets, which always pass
-mbig-endian.

Much thanks for the patch!

Compiler crash reported in: https://bugs.llvm.org/show_bug.cgi?id=48695

This causes:

$ clang -E -fno-integrated-cc1 x.c | tee foo.txt |head

to crash. If I revert 9a3f892d018238dce5181e458905311db8e682f5 and 4624e83ce7b124545b55e45ba13f2d900ed65654, the crash goes away.

It sounds like portage should be using something like setuidgid to drop privileges for parts of the build.

TODO: fix failing tests:

• lld/test/ELF/error-handling-script-linux.test
• llvm/test/ExecutionEngine/JITLink/X86/ELF_x86-64_relocations.s

The commit message of https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f923efbcfdbaa4391874eeda676b08fcf9ad6e99 also has good background/history.

If tlbiel %r4 is equivalent encoding wise to tlbiel %r4, 0, then we could work around this limitation in Clang's integrated assembler via kernel patch:

diff --git a/arch/powerpc/mm/book3s64/hash_native.c b/arch/powerpc/mm/book3s64/hash_native.c
index 52e170bd95ae..3e902dc91582 100644
--- a/arch/powerpc/mm/book3s64/hash_native.c
+++ b/arch/powerpc/mm/book3s64/hash_native.c
@@ -267,7 +267,7 @@ static inline void __tlbiel(unsigned long vpn, int psize, int apsize, int ssize)
                va |= ssize << 8;
                sllp = get_sllp_encoding(apsize);
                va |= sllp << 5;
-               asm volatile(ASM_FTR_IFSET("tlbiel %0", "tlbiel %0,0", %1)
+               asm volatile(ASM_FTR_IFSET("tlbiel %0", "tlbiel %0", %1)
                             : : "r" (va), "i" (CPU_FTR_ARCH_206)
                             : "memory");
                break;

which looks like a mistake in the kernel to not use the same number of operands for either case (though they should encode the same); but I'd much rather improve LLVM if possible.

In D92156#2461763, @jsji wrote:

tlbie is different from tlbiel.

Thanks Jian! (I can get a little further assembling the Linux kernel's arch/arm/probes/kprobes/test-arm.o (allyesconfig) with Clang's integrated assembler with this; though that file now has additional failures unrelated to this patch.

In D92156#2459057, @jsji wrote:

Perhaps GNU assembler has a pseudo-instruction for this? If it's not used significantly throughout the Linux kernel, maybe we should change the kernel to use the canonical form of the instruction?

I did some research and chatted with GNU assembler developers. I think the situation is:
Old ISA used to define 2 operands tlbiel, but we changed the format in ISA 3.0 (Power 9) and later , introducing the 5 operands basic form and its one operand extended mnemonic.

So gas supports the 5 operands basic forms only with -mpower9 and up. The 2 operands mnemonic in gas is actually an extended mnemonic with other 3 operands default to 0.

We are working with ISA guys to confirm whether we can support the 2 operands extended mnemonic.

So my recommendation is we should implement according to ISA 3.1 first for now, we can add the two operands InstAlias later if confirmed by ISA .

Regarding to usage in kernel, I think it depends on which arch are you building for, if it's with -mcpu=pwr9 and above, yes, I would recommend we update the code to use 5 operands basic form.

In D92156#2461086, @jsji wrote:

In D92156#2459303, @nickdesaulniers wrote:

Regarding to usage in kernel, I think it depends on which arch are you building for, if it's with -mcpu=pwr9 and above, yes, I would recommend we update the code to use 5 operands basic form.

I haven't verified yet that -mcpu=pwr9 is being used, but it looks like the one TU that's using tlbie with 2 operands mixes the use of 2 and 5 operand variants without any kind of check from what I can tell. If that's the case, we might be able to do:

Great. Thanks. It would be great if we can get rid of 2 operands usage in kernel...

Thanks for the patch!

Regarding to usage in kernel, I think it depends on which arch are you building for, if it's with -mcpu=pwr9 and above, yes, I would recommend we update the code to use 5 operands basic form.

According to ISA 3.1 https://ibm.ent.box.com/s/hhjfw0x0lrbtyzmiaffnbxh2fuo0fog0 Page 1239 there are only two forms

Thanks for the patch.

In D90271#2436080, @SolarAquarion wrote:

You missed a change https://bugs.llvm.org/show_bug.cgi?id=48413

@lebedev.ri would you mind re-reviewing when you have a chance?

s/MSDN/Microsoft Docs/

I suspect that duplicateInstruction is not updating isInlineAsmBrIndirectTarget correctly when moving the INLINEASM_BR, but will have to verify.

Sorry, I should not have waiting this long to update this patch...

• rebased, s/Inline/Inlining/

In D92695#2435063, @nickdesaulniers wrote:

In D92695#2435025, @craig.topper wrote:

Maybe the linux kernel disables the red zone and this isn't a problem.

The x86_64 (64b) Linux kernel is built with -mno-red-zone; I'm not sure if that's precisely the same as "disables the red zone."

In D92695#2435025, @craig.topper wrote:

Maybe the linux kernel disables the red zone and this isn't a problem.

In D92695#2434973, @craig.topper wrote:

What prevents overwriting something in the red zone?

Hi @trixirt , thanks for the follow up. I think the lint feedback about qualifying auto with pointers where applicable would be nice to have. Please make those suggested changes.

Cool, that should be all of the feedback addressed. How does it look?

• rebase on https://reviews.llvm.org/D92350, use split-file, simplify assert condition, rewrite unclear comment.