In D78793#2424089, @dexonsmith wrote:

In D78793#2424081, @nicholas wrote:

! In D78793#2424013, @dexonsmith wrote:
In particular, it would be nice if LLVMContext were restricted to types / values / metadata that are purely functional, and anything non-functional or open to interpretation is put on the Module.

• For values and metadata, this requires some significant work (I have some concrete ideas of how, but it seems quite tangential to this patch).
• For types, this could be done as a very simple follow-up to the opaque pointer work (@dblaikie @arsenm, not sure if you have thoughts). The Type class would be purely functional, and the struct names would move to a symbol table in each Module. The name is not inherent to the type, it's just a handle that some module wants to use to refer to it (the type itself can be shared between modules even if they disagree about what to call it).

I don't have much to add here, I'd simply recommend that you have a plan for dealing with the problems of type merging and refinement, structs whose members are pointers to that same struct (detect and reject creating a struct that holds itself as a member, %0 = type { \0 }), etc., before making the change.

I'm not familiar with the opaque pointer work and how it affects the types and context.

Yeah, that's exactly what's fixed by the opaque pointer work: it turns the type graph into a DAG. In your example, the type wasn't holding itself as a member (that's not possible), it was holding a pointer to itself as a member. Once pointers are opaque, then it's just holding a pointer, no cycles anymore.

In D78793#2424013, @dexonsmith wrote:

@nicholas, I missed this when it was reviewed, but seeing it now that you've landed it.

Can you help me understand if there's a deeper motivation than the one in the patch description? I ask because I would have envisioned eventually moving these "named" types to the module (i.e., keep the old API before your change, but change the implementation to make sense of it).

Use LLVMGetTypeByName2 in llvm-c-test echo test too.

Could you include a test with this patch?

We can, however, use information from non-exact definitions to improve
themselves as they are either replaced as a whole or not at all.

In D63046#1535804, @nicholas wrote:

The current algorithm can deduce only for very simple cases.

• A function doesn't have any loop.
• A function doesn't have any recursion.

What about functions that exit the program, throw an exception, or longjmp?

The current algorithm can deduce only for very simple cases.

• A function doesn't have any loop.
• A function doesn't have any recursion.

Why did the number of nounwinds drop?

In D60074#1533285, @jdoerfert wrote:

Thanks for the extensive comment!

In D60074#1532046, @nicholas wrote:

In D60074#1531819, @jdoerfert wrote:

In D60074#1531809, @efriedma wrote:

Is it actually correct to derive norecurse like this?

Suppose I have two functions:

void f(int x) { if (x) { g(); } }
void g() { f(0); }

As far as I can tell, it would be correct to mark g() norecurse. And if g() is marked norecurse, we'll mark f() norecurse, which is wrong.

We should/do not mark g as norecurse in this example. At least not if f is externally visible.

Could you explain a little further? I don't see how changing whether f is externally visible changes whether it's OK to mark g with norecurse. Here's my thinking. If f is externally visible that means it can be called from external code, and we have to make the worst-case assumption about the call stack, but in this case we know that g can't be on the call stack since g doesn't call any external functions. Is that right?

I should have explained this better from the beginning. At the moment, for better or worse, we derive norecurse only if the function does not call a recursive function. I did not want to change this behavior in this patch but I'm willing to investigate if we should. Now, in the example above, f is recursive as it calls g which calls f. And g is recursive as it calls f which was recursive. My internal comment was following this logic: If f was internal, we could (conceptually) eliminate the call of g after IPSCCP and thereby making both functions non-recursive.

In D60074#1531819, @jdoerfert wrote:

In D60074#1531809, @efriedma wrote:

Is it actually correct to derive norecurse like this?

Suppose I have two functions:

void f(int x) { if (x) { g(); } }
void g() { f(0); }

As far as I can tell, it would be correct to mark g() norecurse. And if g() is marked norecurse, we'll mark f() norecurse, which is wrong.

We should/do not mark g as norecurse in this example. At least not if f is externally visible.

This update adds a failing test @test21 which demonstrates a miscompile. While we check the block with the free in it for potentially-malloc'ing instructions, we don't examine all the possible paths from where that free was pre-transform to the new location of the free post-transform.

CFG.cpp doesn't have many non-cleanup authors, Bruno once made a change all the way back in 2015 to improve the performance of isPotentiallyReachable. Alina, I've added you since you reviewed my previous change to isPotentiallyReachable when it affected LICM. This series of patches improves performance of isPotentiallyReachable, primary by taking advantage of dominator tree properties.

In D60659#1466869, @reames wrote:

In D60659#1465605, @nicholas wrote:

Why do you think a store through constant would break subtly? For most users I'd expect a store to a constant to correspond with a write to a read-only page causing a loud crash.

I assume you mean before this patch right? If so, then yes and no. It's possible the constant location is in a readonly page, but I have a lot of downstream constant locations which are intermixed with writeable data. I'm sure other frontends might have the same. Your reasoning may hold specifically for global constants though.

The two statements:

We could simply say that the store *is undefined*, and prune the path.

and

The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.

conspire to make me think that pruning the whole path really is the right thing to do. This might be the only hint the compiler has that the path is dead code.

rL357734

Fixed creation of invalid IR when the loop exit block we need to insert a free call in, is a catchswitch block.

In D60056#1450884, @nicholas wrote:

Should I pull it out of hoisting and sinking and into a post-pass like LICM's scalar promotion? Initially I had imagined that it would participate in hoist and sink's queries of whether "are all arguments to this instruction loop invariant (outside the loop)" as hoists instructions iterately, it would hoist malloc like any other instruction. In practice that couldn't happen because the hoist is deferred until sink time.

I've only skimmed through the high level comments so far, so if if any of this is addressed inline, just say so.

I was wondering why you phrased this as a combination of hoisting and sinking as opposed to promotion. We have the scalar promotion path, and the basic transform you're doing feels a lot like promotion. I suspect that many of the legality aspects will be common.

Committed revision 357425.

In D53556#1291564, @eush wrote:

Could you commit this patch on my behalf?

I misspoke, I meant to say please wait for another reviewer. I've been away from LLVM long enough that I don't have enough context to OK patches to land, but I didn't mean that rnk must be the other reviewer. Sorry!

This looks good to me, but please wait for rnk.

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.

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.