Addressing comment from @mssimpso. Thanks!

Hello @jlpeyton @tlwilmar, is there a chance that this breaks runtime/test/ompt/misc/api_calls_from_other_thread.cpp test? ompt_get_num_places() is expected to return 0 from the test but with this patch it returns 1, and I think it is because kmp_affinity_num_masks is set to 1 from kmp_create_affinity_none_places().

Addressing comments from @fhahn and @junbuml's comments. Thank you!

Thanks @fhahn for the comments! I added comments about why we don't perform splitting for the callsites inside the landing pad. Please let me know if you think it is too verbose.

Addressing @junbuml's comments. Thanks!

Update test to check f2.llvm.0 as well.

@tejohnson Thanks for the clarification. Regarding hotness, I'm not sure if providing "some" hotness is better than leaving it as unknown if profile data is not provided (If profile data is given, as you said, VP metadata will be attached to the callsite). I'm afraid that synthesized hotness may confuse optimizers, but please let me know if you have different idea.

@tejohnson I think your right. What I meant was that when the metadata is imported to bar.o, it references f1 and f2 by their promoted names, which makes the declarations with the promoted names to be added. Did I get it right, or still miss something?

clang-format

@jkorous-apple Got it. I agree that it would be better to move the comments to the header. Will land it soon. Thanks!

Thanks @jkorous-apple for the comment. I think your suggestion is a more
precise description for the implementation, and adjusted the comments
accordingly.

@jkorous-apple Thanks for the comments! Yeah, I was thinking of O(lenght_of_string) approach, but considering the complicatedness of the implementation (I guess the real implementation would be a bit more complex than your pseudo implementation to handle quote and '\n\r' '\r\n' cases) I decided to stay with O(length_of_string * number_of_endlines_in_string) but optimizing the number of move operations.

Thanks @jkorous-apple for your comments. I modified the type for the variables and replaced unnecessary inserts and erases with updates.

Addressing @vsapsai's comments. Thank you for the suggestion! Added test case actually finds an off-by-one error in the original patch. I improved the comments as well.

Ping. Thanks!

Ping. Thanks!

clang-format

Ping. Thanks!

Ping, Thanks!

Addressing @davidxl and @xur's comment. Thanks!

Fix a test.

Friendly ping. @davidxl, I think there's no harm to make clang consistent with gcc for compiler options, and I wonder if you have any concerns that I may miss. Thanks!

Ping. Thanks!

Addressing dblaikie's comments. Thanks!

Relocate.

Ping. Thanks!

I think it is generally good to match what GCC does to not to confuse people.

Update documentation. Please let me know if I need to update other documents as well. Thanks!

@davidxl I think it is theoretically possible, if the if branch is not taken on line 294. Did I miss something? Thanks!

Ping. Thanks!

Ping. Thanks!

Delete unnecessary line from the test.

@wmi Good call! I fixed the code per your suggestion. Thanks!

minor fix.

Addressing comments from @wmi. Thank you for the suggestion!

https://reviews.llvm.org/D34796 is clang side change.

Ping. Thanks!

Hello @Ayal, can you please update the comments per @hfinkel's request so that I can accept the patch? Thanks!

Addressing @wmi's concern by limiting the targets to the recurrence cycles that only the last instruction of the recurrence (that feeds the PHI instruction) can have uses outside of the recurrence. This is not an ideal solution yet, and more fundamental solution (such as having recurrence optimization as a separate pass and/or using live range analysis for it) should follow. But still I think it is worth to have it here.

In D34373#785642, @hfinkel wrote:

In D34373#785485, @twoh wrote:

In D34373#785367, @hfinkel wrote:

In D34373#784975, @twoh wrote:

I think this is a right approach, but concerned that the experimental results I shared on D32451 show that it is generally better to not to vectorize the low trip count loops. @Ayal, I wonder if you have any results that this patch actually improves the performance. Thanks!

I know that we're currently missing opportunities for large vectorizable loops with low (static) trip counts. Smaller inner loops are also good candidates for unpredicated vectorization, but we may need to be a bit careful because of modeling inaccuracies and phase-ordering effects (e.g. if we don't vectorize a loop, then we'll end up unrolling it when the unroller runs).

Got it. My concern was for small single-level loops with low trip counts, as I observe them pretty frequently. I have no objection accepting this patch and improve the cost estimator separately.

Do you mean that you see such loops frequently with dynamically-small trip counts, or with static trip counts? I assume the small loops with (small) static trip counts will generally be unrolled.

In D34373#785367, @hfinkel wrote:

In D34373#784975, @twoh wrote:

I think this is a right approach, but concerned that the experimental results I shared on D32451 show that it is generally better to not to vectorize the low trip count loops. @Ayal, I wonder if you have any results that this patch actually improves the performance. Thanks!

I know that we're currently missing opportunities for large vectorizable loops with low (static) trip counts. Smaller inner loops are also good candidates for unpredicated vectorization, but we may need to be a bit careful because of modeling inaccuracies and phase-ordering effects (e.g. if we don't vectorize a loop, then we'll end up unrolling it when the unroller runs).

I think this is a right approach, but concerned that the experimental results I shared on D32451 show that it is generally better to not to vectorize the low trip count loops. @Ayal, I wonder if you have any results that this patch actually improves the performance. Thanks!

Addressing comments from @tejohnson. Thanks!

@tejohnson Sorry for the late reply. I was out of internet for days. I have no objection to set the default value to true, but prefer to have it as a separate patch so that we can track the impact on performance of each chance more easily.

Ping. Thanks!

@wmi Not at all! Thanks for your comments.

ping. thanks!

FYI, below is the compile difference in percentage for spec2006:

Add missing reference.

Reimplement the feature in peephole optimizer. This patch makes the values in the recurrence cycle to be tied to each other if possible.

Ping. Thanks!

Comparing the existing implementation and this patch, I don't observe noticeable compile time different with Video-SIMDBench. I compiled the benchmark suite 3 times each and the median was 27.12 sec vs 27.55 sec, while the average was 27.96sec vs 27.89 sec. There were some code size differences, but it is simply more vectorization results bigger code size. There's no difference between OptForSize and non-vectorized.

Addressing Ayal's comments.

@wmi Thank you for your reply. I agree on you that we should consider tied operand group, and as @qcolombet mentioned in the previous comment, if tie operand information is already available before this pass, I'd like to discuss where would be the best place to implement this.

@qcolombet Actually you're right. Having tied operands, this can be done within SSA. Do you have a suggestion for a better place to implement this? I thought what isRevCopyChain does is most close to what this patch does.

@MatzeB Thank you for your comments. I'm collection compile time numbers now with spec2006. For our internal benchmark, which takes about ~40min to compile, the compile time difference was negligible.

Addressing comments from @MatzeB.

@Ayal Thanks for the pointer.

Ping. Can someone please let me know what would be the best way to have this patch reviewed? Thanks!

I don't have a strong opinion about treating static trip count and profile-based trip count or not. I treated them separately because existing implementation sets OptForSize to true instead of returning false when it makes a profile-based decision. It would be great if someone can explain the reason behind that.