@nikic, this change caused major degradations to our downstream benchmarks. Basically, in the good case, we used to have selects (which got converted to BLENDVPD instruction). I wonder if there's a reverse transform somewhere when we have branches without profile and change them into selects.

Abandoned in favour of D126777.

This patch works for the crashes we saw downstream. Thanks!

simplified test case

In D126713#3548047, @ABataev wrote:

Thanks for the patch.

1. Would be good to check if it affects the vectorization of the SPECs.

Could you pls elaborate? Unfortunately, I don't know what needs to be done here and I think irrespective of any vectorization "performance issue", we would need to fix the *functional problem*.

JFI: It also requires updating a lot of tests, around 25 in X86 with CHECK lines updated since the insertion point changed. I have not tested the other targets, but there would be others as well.

@ABataev, we are getting an assertion failure downstream in setInsertPointAfterBundle which is bisected to this patch and reproducible with it.

went another path.

In D125690#3538597, @aeubanks wrote:

does using the size parameter not work for your use case? do you specifically need the type?

addressed review comments.

already landed. closing as done.

When tested on a large method from our internal workload, I see about 12% reduction in BranchFrequencyAnalysis (other analyses time also dropped to a lower degree). Overall opt time also dropped about 4%.

thanks for the quick review!

In D124115#3462940, @nikic wrote:

In D124115#3462890, @anna wrote:

The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

I'm confused why we cannot have zero values for int attributes? The change here was to specifically allow zero values for any int attribute (because we have a downstream attribute where the value can be zero).
Is this a verifier issue - for example, we cannot have derferenceable_or_null with 0.

I don't think there is a strong technical reason for this restriction, it's just an artifact of the API design. For some int attributes we perform an encode/decode step to translate between a "raw value" (which is never zero) and a semantic value (which can be zero). I do think we should lift this restriction, though I'm not sure if just adjusting this one constructor will be sufficient.

In D124115#3462855, @nikic wrote:

I would prefer to split this into two overloads, one without the uint64_t argument creating an enum attribute and one with creating always an int attribute, and fix up any problematic uses accordingly. The current approach leaves behind a footgun if we start actually using zero values for int attributes (something that we are careful not to do currently).

Do we have any compile time measurements for this change with O3?

LGTM with a comment. Great job on root causing and fixing this!

I spend sometime trying to see if the failing premerge tests seem related or maybe false positives, but I have no idea where these are from. Also, other completed unrelated review show the same failures: https://reviews.llvm.org/D94928

support system and singlethread syncscopes. Added test for target-dependent syncscope to showcase need for isIdenticalTo check.

addressed review comments. Added extra tests.

fix based on Philip's comments about scopes.

already handled as needed.

Looks like the comments have been addressed regarding the split. Thank you! Moving myself out of blocking state.

At this point, everything necessary is landed upstream through the different approach. See linked patches from Philip.

In rG4d58d1d5af31eb386b73279f288286db225fabae#1047741, @dblaikie wrote:

In rG4d58d1d5af31eb386b73279f288286db225fabae#1047676, @anna wrote:

There are 14 test failures in trunk, which disappear once this change is reverted. I wonder why the Buildbot didn't catch it.
Here's 13 of them with the .yaml extension:

Failed Tests (13):
  LLVM :: tools/llvm-dwarfdump/X86/verify_curanges_incomplete.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_die_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_cu_ref.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_die_range.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ref_addr.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ref_addr_between.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_rnglists.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_stmt_list.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_strp.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_lexical_block_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_overlapping_function_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_overlapping_lexical_block_ranges.yaml

Once the change is reverted, these tests pass. @dblaikie could you pls fix? It just needs testcase updates to add the Verifying unit: line (at least for the few I manually checked).

Are you testing with current HEAD? A bunch of these were fixed in a follow-up commit 09f43c107fc7688639346d3beead72472cdadbdb

There are 14 test failures in trunk, which disappear once this change is reverted. I wonder why the Buildbot didn't catch it.
Here's 13 of them with the .yaml extension:

Failed Tests (13):
  LLVM :: tools/llvm-dwarfdump/X86/verify_curanges_incomplete.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_die_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_cu_ref.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_die_range.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ref_addr.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_ref_addr_between.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_rnglists.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_stmt_list.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_invalid_strp.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_lexical_block_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_overlapping_function_ranges.yaml
  LLVM :: tools/llvm-dwarfdump/X86/verify_overlapping_lexical_block_ranges.yaml

@spatel: inline comment added about incorrect arguments for one of the callsites. Pls fix.

Good catch!

LGTM

In D109917#3188468, @nikic wrote:

I see that you've added the writeonly argument handling to this patch. I would prefer to split that off into a separate one, because it's not directly related. Even if it means that this lands without any upstream test changes at first.

I don't think we generally do this. I will need to add either the allocations or the paramcase soon-ish after this. But I see your point (since the change as-is does not affect any upstream code). Will wait if @reames has any concerns.

Note that this is a rewrite of original patch to handle sinking of function call with outparam(s). I have also added allocation test cases to show that these are not affected by the patch.

rewrote for outparam usecase in simplest form

Change LGTM. @jdoerfert any additional comments?

LGTM.

In D115362#3180931, @reames wrote:

In D115362#3180798, @anna wrote:

Nice find! This is obviously a good thing to do for unrolling. Any ideas why there's no vectorization test cases to update since we do support multi-exit vectorization?

Well, purely because I didn't write one. The unrolling test change didn't exist either before I wrote the one showing a diff this morning. Note that even that one requires a non-default flag value to show up.

I don't think the change in vectorization is likely to be interesting, and crafting a test will be tedious. Do you want me to bother?

Nice find! This is obviously a good thing to do for unrolling. Any ideas why there's no vectorization test cases to update since we do support multi-exit vectorization?

Actually looking at the patch again, you're mixing two concepts here:

1. fixing the LangRef and Attributes.cpp for align attribute on vector of pointers should be done as an independent patch. It is orthogonal to the SelectionDAGBuilder code and avoids the false dependency created by the parent patch linked to this patch.
2. ABI intrinsics and SelectionDAG builder changes can depend on the parent patch.

In D114423#3173134, @frasercrmck wrote:

Aha I just found D87304 - maybe @jdoerfert @fhahn @anna @dantrushin would be interested in this patch.

In D109917#3165403, @reames wrote:

Thinking about it a bit, I think there are some interesting cases where we can sink something with an analyzeable side effect. An example I came up with is a large function with out-params which are unused in the caller.

Thanks Philip. This is a definitely interesting example worth handling. Let me update the code to consider this use case and precommit some of the tests. The use case we have downstream (as mentioned is object.hashCode), but this example makes a good usecase for upstream IMO.

In D109917#3165100, @nikic wrote:

Do you have a use case for this apart from sinking allocations? If not, I would recommend restricting this to allocations only, because the generalized reasoning seems pretty subtle to me, and I'm not sure in which cases (apart from allocations) it would help.

updated to correct diff.

rebased

rebased

addressed review comments

separated out API

This change looks good to me and thank you for taking this through!

addressed review comment about naming. Updated a comment in the testcase to parse better.

In D111668#3071306, @asbirlea wrote:

Looks reasonable, assuming you're seeing similar results to using BPI. I'm not very familiar with BPI itself, so if others reviewers are, I'll let them chime in.

Does it make sense to make isValidProfileData and ComputeBranchProbability static utilities in BranchProbability? (nit: consistent case-ing on the names)

I think that's a good idea. Will do as a follow-up after landing patch.

ping

minor changes in naming, clang format. NFC w.r.t. previous version.

In D111448#3052089, @asbirlea wrote:

I'm confused how the intermittent assert occurs. Is the compilation non-deterministic?