In D83573#2281432, @danielkiss wrote:

Sorry for the trouble for now the patch is reverted.
I you agre I will resubmit it with ; REQUIRES: x86_64-linux and then figure out the rest of the targets later.

Thanks,
Daniel

I landed this on behalf of Gabriel.

Having some problems after this patch, when building the runtimes for i386, on a 64-bit RHEL7 server, and then running the added libunwind test cases on the same host.
No expert here, and maybe that isn't a valid scenario, but building/running tests like that has worked pretty good before.

Just want to clarify that my questions regarding "what about debug info" in the earlier patch introducing these phi-type-rewrites actually was about dbg.value rather than the debugloc:s. I kind of wanted to have test cases verifying that the the presence of dbg.value uses didn't impact the optimization (-g invariance). But also to see that the dbg.value uses were handled somehow (either being salvaged in some way or being properly invalidated).

Just hoping that those unused functions aren't used by some OOT project. But considering that the API is totally untested (and the unwind part isn't even visible any longer) I think this cleanup is motivated.

Things seem to link again for us if we set LLVM_ENABLE_TERMINFO=OFF (as a workaround to avoid dependency to libtinfo), but I guess we might lose some color output or something by doing that.

So are we perhaps missing tinfo (and ncurses) in /proj/bbi_twh/wh_bbi/x86_64-Linux2/bbigcc/1.7.4.0-3/crosscompiler/lib64?
I doubt that we should pick up libtinfo from /usr/lib64 here, even if we do not really need to crosscompile llvm-tblgen (tblgen should only execute on the current host anyway).

I don't really understand the problem situation. Why are you having two source operands if they need to be the same? Can't you ensure that by only having one source operand?

Should I land this patch for you (including a fix for the long lines)?

@ehjogab : Should I help out landing this patch?

In D86472#2234113, @lebedev.ri wrote:

@bjope thanks! bugreport kinda skimmed my attention span, i was going to re-look today-ish.
I think this looks fine. The another alternative would be to only do that if the value-to-be-renamed is an instruction,
but then i'm not sure it's an improvement if we rename something that we reused.

Proposed fix for the problem described by @materi : https://reviews.llvm.org/D86472

LGTM (except the earlier nit about the comments with different X and Y values on left and right hand side)

I've submitted a PR, https://bugs.llvm.org/show_bug.cgi?id=47296, regarding the debuginfo problem.

In D86430#2232959, @foad wrote:

In D86430#2232951, @foad wrote:

It could be possible to add back the transform, given that logic
is added to check that (Z % BW) can't be zero. Since there were
no test cases proving that such a transform actually would be useful
I decided to simply remove the faulty code in this patch.

I don't understand this. The AMDGPU test changes clearly show that the transform is useful. AMDGPU has an alignbit instruction which is fshr, but no instruction for fshl.

Oh I see, I guess you mean that the check for non-zero Z didn't help any of these test cases, because none of them use a shift amount that is known to be non-zero.

OK then. I can try to fix the AMDGPU regressions as a follow up, maybe with a target-specific lowering.

Haven't seen any gains from this patch, after having rebased to https://reviews.llvm.org/rGec06b381304140b2553cfdfae5a063f39c5c59ff .
The test I ran included bunch of application code, compiled with -O3, for our OOT target.

In D86243#2227038, @xbolva00 wrote:

Some SCEV/pipeline tests?

Maybe from https://reviews.llvm.org/D85969#change-8DkHYel6Y1JK ?

I've verified that this resolves https://bugs.llvm.org/show_bug.cgi?id=47136 in the sense that our downstream benchmark result is back to the original result for the tests that had a regression. And without any new regressions. So that is perfect!

I do not have any upstream test case for this, but got failures in our downstream testing.

One minor problem with implementing a reverse transform for targets that prefer sext over zext, e.g. in CodeGenPrepare, is that it will be hard to restore the debug info (considering that we do the right thing here an invalidate metadata uses).

In D60413#2223875, @lebedev.ri wrote:

In D60413#2223866, @bjope wrote:

Having some problems with this patch downstream:

1. Patch does not update any dbg.value that might refer to the value that now is zext:ed instead of sext:ed. So debugging experience might become weird. It might be possible to rewrite debug uses, using some nifty debug expression to express a trunc+sext of the value. But in my case it was an <128 x i40> vector, and we got no dwarf expressions to express a vector sext afaik. Instead I guess any debug uses should be invalidated.

Isn't that an issue for any of the other places that do such a transform, among other things?
This very transform also exists in InstCombine and CVP.

Having some problems with this patch downstream:

In D85969#2218048, @spatel wrote:

I don't have much experience with SCEV either, but I think your draft change for instcombine made sense:
https://bugs.llvm.org/show_bug.cgi?id=47136#c8

As hinted there, that's the same direction as:
D32255 / rG23bd33c6acc4

So I'm not sure if we would consider this a worthwhile improvement independently and still make a change to SCEV or only change instcombine (or if there's some reason to *not* change instcombine, I'd like to understand that).

In D80525#2215778, @adamcz wrote:

Looks like it's an issue with not clearing the module cache. I'll revert this change to fix the build, then re-submit with proper fix.

Might be that it is some other commit that cause the problem. Looking a bit closer at the jenkins history it seems like the tests passed once after this patch was added. But the next time it failed.

In D80525#2215641, @adamcz wrote:

This is quite strange. I can't reproduce this with gcc 7.5.0, 9.3.0 or 10.0.1. Which version are you using?

There are bugs around clangd + modules + diagnostics. I'm fixing one in https://reviews.llvm.org/D85753 (not in it's final form yet), but it doesn't seem to be the same issue (can't be sure, but I think this is using the right SourceManager). This test is not supposed to generate any diagnostics, so that's the really surprising bit. Are you building from clean HEAD, or do you have any local modifications that could generate a extra diagnostics in this test?

The other option is that the failure is caused by module_cache_path. This test, unlike other clangd tests, needs real file system (to write the .pcm file and then read it back) and perhaps that doesn't work in your jenkins setup.

Are there any special options or things unique to your setup I could try out to reproduce this?

In D85446#2212757, @bjope wrote:

In D85446#2205778, @lebedev.ri wrote:

In D85446#2205616, @bjope wrote:

(Just some heads up, in case someone else got the same problem...)

I've noticed some regressions downstream after this patch. Haven't debugged/reduced it completely yet, but at least in one case I suspect that the problem actually might be in load-store-vectorizer.

Used to get something like this before load-store-vectorizer:

<...>

The old input to load-store-vectorizer resulted in two <8 x i16> loads, but the new input gives one <8 x i16 load, two <2 x i16> loads and one <4 x i16> load. So for some reason LSV fails to detect that the loads are consecutive given the new IR. At least that is my current theory.

Please can you file a bug with reproducer?

Yes, I'm working on that (although progress has been a bit slow at the moment).

In D85446#2205778, @lebedev.ri wrote:

In D85446#2205616, @bjope wrote:

(Just some heads up, in case someone else got the same problem...)

I've noticed some regressions downstream after this patch. Haven't debugged/reduced it completely yet, but at least in one case I suspect that the problem actually might be in load-store-vectorizer.

Used to get something like this before load-store-vectorizer:

<...>

The old input to load-store-vectorizer resulted in two <8 x i16> loads, but the new input gives one <8 x i16 load, two <2 x i16> loads and one <4 x i16> load. So for some reason LSV fails to detect that the loads are consecutive given the new IR. At least that is my current theory.

Please can you file a bug with reproducer?

(Just some heads up, in case someone else got the same problem...)

LGTM!

In D83216#2196048, @ebevhan wrote:

In D83216#2196028, @lebedev.ri wrote:

LGTM, but i would have preferred to see more feedback on the RFC thread.
That being said, unless you are planning on forming calls to these intrinsics in middle-end transform passes
(i.e. only planning on using them in clang codegen), i think this is okay to proceed.

At least for now, I'm not planning to emit them in middleend. I wanted to get most of the basic support in Clang and LLVM before looking at optimization.

However, based on the discussions in D82663, I am planning on sending an RFC for moving the fixed-point semantics and value class to LLVM, and adding a Builder class similar to the MatrixBuilder.

In D85191#2196863, @rsmith wrote:

In D85191#2195923, @ebevhan wrote:

In D85191#2193645, @rsmith wrote:

This is not ideal, since it makes the calculations char size dependent, and breaks for sizes that are not a multiple of the char size.

How can we have a non-bitfield member whose size is not a multiple of the size of a char?

Downstream, we have fixed-point types that are 24 bits large, but where the char size is 16. The type then takes up 2 chars, where 8 of the bits are padding. The only way in Clang to express that the width of the bit representation of a type should be smaller than the number of chars it takes up in memory -- and consequently, produce an i24 in IR -- is to return a non-charsize multiple from getTypeInfo.

This violates the C and C++ language rules, which require the size of every type to be a multiple of the size of char. A type with 24 value bits and 8 padding bits should report a type size of 32 bits, just like bool reports a size of CHAR_BIT bits despite having only 1 value bit, and x86_64 long double reports a type size of 128 bits despite having only 80 value bits.

Thanks for the fixes Dave. Will make sure that we start using this in our (downstream) fuzzy testing again.

LGTM!

I have no clue if this has any negative impact from the https://reviews.llvm.org/D84040 point-of-view, etc. Maybe @ldionne can take a quick look at this as well (pre/post commit).

In D84132#2164675, @MaskRay wrote:

In D84132#2164644, @bjope wrote:

Having some problems with failing stage 2 tests, that I suspect is related to this change (since the missing files do exist but with ".lto" being part of the name when looking in my test directory after a failed test run). I guess on need to add ".lto" at some more places?

These are the tests that failed:

Failed Tests (3):
  LLVM :: tools/gold/X86/cache.ll
  LLVM :: tools/gold/X86/emit-llvm.ll
  LLVM :: tools/gold/X86/relax-relocs.ll

Here are some logs:

: 'RUN: at line 70';   ls /build/stage2/test/tools/gold/X86/Output/cache.ll.tmp4.o.o1
: 'RUN: at line 71';   ls /build/stage2/test/tools/gold/X86/Output/cache.ll.tmp4.o.o2
--
Exit Code: 2

Command Output (stderr):
--
ls: cannot access /build/stage2/test/tools/gold/X86/Output/cache.ll.tmp4.o.o1: No such file or directory

Exit Code: 2

Command Output (stderr):
--
/build/stage2/bin/llvm-nm: error: /build/stage2/test/tools/gold/X86/Output/emit-llvm.ll.tmp3.o.o: No such file or directory.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /build/stage2/bin/FileCheck --check-prefix=NM /llvm-project/llvm/test/tools/gold/X86/emit-llvm.ll

Exit Code: 2

Command Output (stderr):
--
/build/stage2/bin/llvm-readobj: error: '/build/stage2/test/tools/gold/X86/Output/relax-relocs.ll.tmp.so.o': No such file or directory
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /build/stage2/bin/FileCheck /llvm-project/llvm/test/tools/gold/X86/relax-relocs.ll

Thanks for reporting the problem. Fixed by aa830e9768303ff8d27c015759294c4ce704d50c (I should have been more careful. Not many build bots build binutils-gdb headers and test LLVMgold.so)

Having some problems with failing stage 2 tests, that I suspect is related to this change (since the missing files do exist but with ".lto" being part of the name when looking in my test directory after a failed test run). I guess on need to add ".lto" at some more places?

I'm a bit curious about the overall plan here. Some years ago the plan was to remove LiveVariables, and use LiveIntervals instead. And afaik PHIElimination was updated to support both LiveVariables and LiveIntervals, while TwoAddressInstructionPass only got partially updated. So for several years there have been a technical debt here, considering that the passes involved in de-SSA to some degree are supporting both LiveVaribels and LiveIntervals, while only LiveVariables is used in practise (and the support for LiveIntervals may have rotten since it is poorly tested).

Here is a reproducer for the new problem that @uabelho noticed. Might be possible to reduce it further, but I think it at least show the problem:

In D83101#2136116, @lebedev.ri wrote:

Ah, maybe I was so occupied reducing the fault so I missed that there was another fix. I actually did take a look in github just to avoid reporting a problem that had been fixed, but must have done it just before rG16266e63963ad6ee27ad21983a9366ab313dfd03 landed.

Nono, rG16266e63963ad6ee27ad21983a9366ab313dfd03 is the fix as a reaction to the bug you reported, your report wasn't a duplicate, sorry for confusion.

In D83101#2136062, @lebedev.ri wrote:

In D83101#2135962, @lebedev.ri wrote:

In D83101#2135945, @bjope wrote:

I unfortunately still see some problems (related to the Scalarizer changes, and probably this patch):

> cat scalarizer-bug.ll
; RUN: opt < %s -scalarizer -S -o -

define void @foo() {
vector.ph:
  br label %vector.body115

vector.body115:                                   ; preds = %vector.body115, %vector.ph
  %vector.recur = phi <4 x i16> [ undef, %vector.ph ], [ %wide.load125, %vector.body115 ]
  %wide.load125 = load <4 x i16>, <4 x i16>* undef, align 1
  br i1 undef, label %middle.block113, label %vector.body115

middle.block113:                                  ; preds = %vector.body115
  ret void
}


-----------------------------------------------------

> ~/opt.master -scalarizer scalarizer-bug.ll -S
opt.master: ../lib/IR/Value.cpp:458: void llvm::Value::doRAUW(llvm::Value *, llvm::Value::ReplaceMetadataUses): Assertion `!contains(New, this) && "this->replaceAllUsesWith(expr(this)) is NOT valid!"' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: /home/uabbpet/opt.master -scalarizer scalarizer-bug.ll -S 
1.      Running pass 'Function Pass Manager' on module 'scalarizer-bug.ll'.
2.      Running pass 'Scalarize vector operations' on function '@foo'
Abort

Acknowledged, looking.

Hm, this is saddening. I've fixed it in rG16266e63963ad6ee27ad21983a9366ab313dfd03, but are there more?