LGTM

In D68615#1708377, @JonChesterfield wrote:

Ping. I'm hoping this is uncontentious.

Small nits. Otherwise LGTM.
Also, make sure we have a test with 2 pointer arguments, one freed, one no-free annotated.

In D29011#1708103, @lebedev.ri wrote:

In line with a few recent reverts, is this missing anything for ocaml binding?
(does it compile with ocaml binding enabled and do all tests still pass?)

Fwiw, I'm OK with this (given that we have my requested changes in upcoming commits).

Minor updates and fixes

quick feedback

Rerun with fixed script to use argument variable names in body

Handle pointers with address spaces and make sure uses are pointer operands

Non-exact functions fix + more tests

Dropped in favor of D68925.

Fixes and check line update

Two minor comments from my side.

In D68626#1707223, @sstefan1 wrote:

I have no problem with this going in as is. I have one question though. Why do we have to wait for ValueSimplify to finish? Wouldn't it be useful for AAIsDead to have isDeadArg(Arg) and then ValueSimplify could use that to decide whether to simplify or not?

In D68819#1705424, @greened wrote:

In D68819#1704962, @jdoerfert wrote:

We can, or should, combine D68153 and this, either in one or two patches.

Sounds good. Do you think D68153 should operate under the --function-signature flag, under a different flag or always include define in the pattern (meaning all tests will change when the tool is re-run)?

In D68766#1704577, @lebedev.ri wrote:

If you want to add attributor runlines, i really insist on following the example i showed, it will result in cleaner diff overall.
That being said i like that the attributor runlines are in a separate diff.

I went through all the tests, added mem2reg/sroa where approriate and modified the source sometimes, mostly to avoid UB. I think the results of the Attributor look good, all problems should have been addressed already.

I stole this and added it into D68819 as well.

Rerun with D68850 and updated D68819

Lessons learned by running it on all argument promotion files: D68766

Lessons learned from running *all* argument promotion tests, patches will follow

In D68153#1698833, @greened wrote:

In D68153#1689791, @RKSimon wrote:

Wouldn't this mean that every regeneration would see this change?

Yes. The label pattern is just wrong as-is because it will match calls in some cases.

In D68819#1704693, @greened wrote:

Does this subsume the goal of D68153? If so I am happy to abandon that revision. D68153 attempts to solve the problem of a CHECK-LABEL matching a function call instead of the start of a function definition. It looks like with --function-signature the CHECK-LABEL will include the arguments in the label pattern which should be enough to disambiguate it from a call to the function. Do I have that right?

EDIT: Actually, I don't think it will completely work if there is a recursive call to, say, foo that passes the same arguments through. In that case the call will look exactly like the function signature. The same is true if an unrelated function makes a call to foo with values that just happen to be named the same as foo's arguments.

Include personality, fix versioning based on different args, tested on D68766

Rerun update with newest version of D68819

In D68819#1704491, @lebedev.ri wrote:

This doesn't change the default, right?

In D67986#1703031, @xbolva00 wrote:

In D67986#1702901, @MaskRay wrote:

This transformation seems to increase code size significantly. Is the snprintf "%s" pattern common enough? I suspect most projects have already used memccpy, stpncpy, strscpy, or strlcpy. For the few that don't, the performance probably does not matter.

Yes, quite common. But okay, if you dont want it, let's just abandon it.

Thx for the initial feedback, I'll update this soon.

Fix all but 3 tests, unknown what the problem there is

I just run update on all of them and did not fix them yet. Please provide feedback if you have any, I'll update this revision asap.

Update tests, keep it in ValueSimplify for now

Add no-alias restriction and test

In D68706#1701937, @spatel wrote:

In D68706#1701915, @jdoerfert wrote:

Can you add the test I provided as well?

Did I miss a message? I copy/pasted at line 92 of the test file (no diff from the code change):

define float @matching_scalar_smallest_deref_addrspace(<4 x float> addrspace(4)* dereferenceable(1) %p) {
; CHECK-LABEL: @matching_scalar_smallest_deref_addrspace(
; CHECK-NEXT:    [[BC:%.*]] = getelementptr inbounds <4 x float>, <4 x float> addrspace(4)* [[P:%.*]], i64 0, i64 0
; CHECK-NEXT:    [[R:%.*]] = load float, float addrspace(4)* [[BC]], align 16
; CHECK-NEXT:    ret float [[R]]
;
  %bc = bitcast <4 x float> addrspace(4)* %p to float addrspace(4)*
  %r = load float, float addrspace(4)* %bc, align 16
  ret float %r
}

Can you add the test I provided as well?

In D68706#1701551, @lebedev.ri wrote:

You want llvm::NullPointerIsDefined(), which also checks for "null-pointer-is-valid" attribute.

I like this better. I'll review it tomorrow (I hope).