Unfortunately this commit causes a serious regression in SSP. I have raised bug 43308:

Updated test3 and test3_errno to check that sinl+cosl does not get transformed into sincosl.

Added additional test from review comments.

In D60793#1471294, @evgeny777 wrote:

what's wrong with keeping it like it is?

Well, if it were possible to avoid the same computation for load I'd like to avoid it. However your patch seems to also handle cases when store is using GEP and load is using bitcast:

So let's stick with current approach. I'd probably include the example above as a test case.

In D60793#1470484, @evgeny777 wrote:

Can't operand 0 of bitcast (%union.A* %u) be used as a key?

In D60793#1470376, @evgeny777 wrote:

"MutatedMemory" is only used to hold evaluated store values

It is also used to patch initializers - see BatchCommitValueTo

In D60793#1469812, @evgeny777 wrote:

I wonder if it's possible to stop using MutatedMemory for checks in ComputeLoadResult and instead start using separate map which can also be filled
in EvaluateBlock. In this new map you can use bitcast (or even first operand of bitcast) as a key.

Addressed review comments.

In D58391#1435174, @tejohnson wrote:

Sorry missed your earlier update. LGTM. Thanks for doing the measurements!

Ping.

In D58391#1408322, @tejohnson wrote:

In D53519#1273387, @rnk wrote:

I'd just land it as is. I think you're right, there's no reason to extend this with a local ad-hoc list, since those other two known intrinsics are unlikely to appear here.f

In D53519#1273358, @rob.lougher wrote:

In D53519#1273062, @probinson wrote:

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I have a memory that somebody wanted an expands-to-nothing property, or generates-no-instructions, or something along those lines. That's probably the kind of generic property you're asking about here? I don't remember the context or outcome, unfortunately.

See my previous comment about ad hoc checks of intrinsic IDs in stack protector, loop vectorizer, etc.

In D53519#1273062, @probinson wrote:

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

I have a memory that somebody wanted an expands-to-nothing property, or generates-no-instructions, or something along those lines. That's probably the kind of generic property you're asking about here? I don't remember the context or outcome, unfortunately.

In D53519#1272775, @rnk wrote:

Is there some more general property for us to check that this intrinsic shares with other annotation-like intrinsics like llvm.assume?

Committed in rL343986 (apologies, I forgot the "Differential Revision:" tag)

I've updated the patch with the following changes:

In D52895#1256101, @rob.lougher wrote:

In D52895#1255911, @nicholas wrote:

In D52895#1255738, @rnk wrote:

Nice!

In D52895#1255602, @nicholas wrote:

I don't understand the logical model behind byval, but I'll review the change to TRE under the assumption that the goal of the patch is correct.

You have to hallucinate an implicit memcpy into the call sequence. Its destination is some stack memory allocated during the call sequence not visible from LLVM IR.

How long does this pointer live? In a sequence with two byval calls on the same pointer, can they reuse the same memory, or it is always reallocated even if it happens to have the same address and contents?

Hi Nicholas and Reid,

In D52895#1255534, @xbolva00 wrote:

This solves https://bugs.llvm.org/show_bug.cgi?id=38862, right?

Added asserts to TTI functions to catch misuse.

I've created a follow up review D50798.

In D48193#1134868, @craig.topper wrote:

This patch tries to fix improve the SVML calling convention. https://reviews.llvm.org/D47188 Maybe it will help this code?

In D48193#1133104, @hsaito wrote:

In D48193#1133083, @rob.lougher wrote:

In D48193#1133065, @hsaito wrote:

I see this as a register allocator problem. It's not like we are running out of registers so that we cannot use ymm0 as a "scratch register" for SVML call. We should show the ASM code to CG experts and get the problem fixed there.

Assuming that register allocator can fix simple enough issues..... I don't think it's correct to model this as a VECLIB call problem. It can happen to any function call that use too many registers that can't be shared among interleaved calls. Instead of looking at whether it's a VECLIB call or not, we should be checking how many registers are used for call/return, and how many of them cannot be shared among interleaved calls. We can then formulate this into a general register pressure issue.

The call uses one register but causes all other live values at the call location to be spilled (the other live values are caused by the interleaving). To model this we would need to know the calling-convention of the target (which registers are preserved), and also which registers the live values will end up in. This isn't known until after register allocation.

Craig topper told me that LLVM currently doesn't have any special knowledge on SVML's register usage. I just checked ICC behavior. ICC uses reg-reg move. So, there appears to be a room for improvement in that area.
I think it's better to look into that first. Doing something blindly like this look easy but I hate to see unnecessary restrictions to be imposed. Even if we don't have a great accuracy here, we should still try to do some accounting, especially so if the calls are to something that's better behaving than the worst case.

What's the value of Legal checking MayHaveVectorLibCall()? Cost model can tell whether the call is vector or scalar on its own for each VF, and that's already happened by the time selectInterleaveCount() is called.

In D48193#1133065, @hsaito wrote:

I see this as a register allocator problem. It's not like we are running out of registers so that we cannot use ymm0 as a "scratch register" for SVML call. We should show the ASM code to CG experts and get the problem fixed there.

Assuming that register allocator can fix simple enough issues..... I don't think it's correct to model this as a VECLIB call problem. It can happen to any function call that use too many registers that can't be shared among interleaved calls. Instead of looking at whether it's a VECLIB call or not, we should be checking how many registers are used for call/return, and how many of them cannot be shared among interleaved calls. We can then formulate this into a general register pressure issue.

In D48193#1133031, @rob.lougher wrote:

In D48193#1132940, @dcaballe wrote:

Hi Robert,

In D48193#1132940, @dcaballe wrote:

Hi Robert,

thanks for bringing this up! This approach is blindly setting the interleave factor to 1 when there are vector math function calls. I have the following questions/comments:

In D46599#1093244, @rob.lougher wrote:

I'll be interested to see what the lexical scopes are for your testcase in PR37149.

Have you ran the LLVM regression tests? In particular, does the test added in D35953 (test/DebugInfo/X86/live-debug-variables.ll) still work as expected?

This is cut and pasted from an email reply as it's not showing up after over 2 hours...

This was reverted in r313057 as it was causing buildbot failure (lldb inline stepping tests).

Address review comments. Now enabled for non-inlined variables. This required fixing a test to make it more resilient (it checks that we get the expected number of debug values, but was also enforcing a specific order). I've also changed where the LexicalScopes are generated, as they're only needed when computing the intervals.

In D28521#644148, @aprantl wrote:

Added a couple of inline comments. LGTM with these changes applied.

In D27590#617635, @jmolloy wrote:

LGTM too, FWIW!

Ping. Adrian, are you happy with this revision?

In D27590#617630, @dblaikie wrote:

I have updated the patch to add a new API for merging debug locations. As explained in the comments, the API is currently a stub which simply uses an empy location.

Just a quick note to say I haven't forgotten about this.

Thanks for all the comments. I'm away for the next couple of days - I'll check back on Monday.

No update. I looked into handling the single line if-then-else case and concluded it was not easy (as Dehao says at this stage you do not know the maximum discriminator value, without redoing the work of AddDiscriminators). As others want the patch could I add a TODO for now?

In D26256#586214, @aprantl wrote:

Shouldn't it only drop the location if the two locations are distinct (and perhaps add a discriminator)?

In D26256#586214, @aprantl wrote:

Shouldn't it only drop the location if the two locations are distinct (and perhaps add a discriminator)?

I've had to revert this as it caused a ubsan test to unexpectedly fail (vptr.cpp). As I don't know much about these tests I need to do some investigation into the failure. I suspect the test was reliant on a debug location from a common-tail.

ping...

In D25742#573702, @probinson wrote:

In D25742#573687, @aprantl wrote:

In D25742#573401, @probinson wrote:

In D25742#573352, @aprantl wrote:

This approach seems generally fine, but I have one question:

If the code were on a single line, and both locations share a common ancestor scope, it seems make sense to create a new location using the common ancestor scope and line and only remove the column information.

That would collapse the if-then-else into (effectively) a single statement. That probably works okay for a debugger but not profiling, which still wants to treat the then/else as distinct. And, after the tail merging, the tails are no longer distinct.

I'm not sure I understand your point. How is having an orphaned add instruction preferable over having it associated with the collapsed if-then-else statement? Wouldn't I want that instruction to be counted towards the line?

No, the profiler wants to assign sample counts to each block individually. By giving each block the same source attribution, you assert that they have the same profiles. That's unlikely to be true in practice. Really what happens is that the sample counts would be assigned to the parent block, which doesn't help the profiler sort out what to do with the nested blocks.

Admittedly, zapping the source attribution on the merged (parts of the) blocks doesn't let you attribute those counts to *any* block, but at least you aren't attributing counts incorrectly.

No problem. Thanks for the code review.

Ping.

Ping. Please can somebody review this? Thanks!

Ping. Just test changes - OK to commit?