Redundant after https://reviews.llvm.org/D150796 and https://reviews.llvm.org/D153768

ping

Hi, this patch is creating malformed IR for some cases. Test case here: https://github.com/llvm/llvm-project/issues/62756

@nikic Please feel free to take over/commandeer this revision.

ping

ping

Hi, I am seeing an 8% degradation in SPEC 2006 Soplex after this patch. Created an issue here: https://github.com/llvm/llvm-project/issues/62100

Added an exception for constant dereferenceable pointers and changed canReplacePointersIfEqual to not look at the Uses.

Hi, I am seeing some inefficient codegen after this patch for AArch64 SVE, created a github issue: https://github.com/llvm/llvm-project/issues/61742

ping

• Use diagnostics (reused existing diag)
• Add a test

In D146333#4204007, @awarzynski wrote:

We already have an error message, so print it as well.

I'm not sure I follow - can you elaborate? This patch is switching from an assertion to a diagnostic. The error message inside the assert will be printed regardless of whether a diagnostic is used or not.

• Guard addToLeaderTable with the pointer replacement check.

There is an existing test flang/test/Driver/target-cpu-features.f90 added in D137995

In D142420#4184825, @tblah wrote:

In D142420#4080619, @mnadeem wrote:

We have tried full LTO on Aarch64 (without this patch) and have seen a few 3-8% improvements in SPEC and a 100+% improvement in leslie3d.
There were a couple of additional failures in SPEC that we have yet to inspect but AFAICT no issue with respect to bitcode/assembly changes.

Hi @mnadeem, thanks for this patch. I'm having some trouble reproducing the improvement on leslie3d. Can you share more information about your configuration? Which flags did you use?

Thanks awarzynski, I have been a bit busy but I will try to land this today after making the test changes.
In the future I'll try to communicate any planning beforehand, using discourse, to make reviewing easier. (I'm still a relatively new contributor to LLVM so still learning :) )

In D142420#4164247, @awarzynski wrote:

[nit]

After this patch:

Do you mean "after this patch lands, I will work on this things ..." , or "after this patch lands, the following becomes available"? You could disambiguate by replacing with "With this patch:", wdyt?

update comments in the test and add newline at the end

Fix the check for PHI nodes and add some reduced tests relevant to the case.

Ported all changes to the patch I previously added as the child revision.

Removing from reviewer's ready to review list for now. Will come back to this patch when/if time permits.

Removing from reviewer's ready to review list for now. Will come back to this patch when/if time permits.

Removing from reviewer's ready to review list for now. Will come back to this patch when/if time permits.

Removing from reviewer's ready to review list for now.

Add LTO bitcode test.

clang format

• Added a new method replaceDominatedUsesWithIf() that takes a callback.
• Updated canReplacePointersIfEqual() to return true for the following:
  • Uses in ICmpInst and PtrToIntInst
  • Replacements with null
  • When underlying objects are the same
• Update tests.
• New LLVM test suite bitcode diff using O3 (assembly files were more difficult to analyze): https://github.com/UsmanNadeem/vigilant-octo-journey/commit/65a770a65eca4ed2cb37f6ff6a36ec6193fe8b73

In D143129#4125424, @nikic wrote:

Here are the diffs for llvm-test-suite bitcode:
https://github.com/UsmanNadeem/vigilant-octo-journey/commit/125698f591df1f8de81ad4119b0838e135a18af8
and
https://github.com/UsmanNadeem/vigilant-octo-journey/commit/a778479334852e7f05b5a248da4473106aa986fb

...
Oh, that's a nice way to analyze differences. How did you produce the output?
...

@nikic
I was looking at https://llvm.org/devmtg/2018-04/slides/Lopes-Sbirlea-Pointers,%20Alias%20and%20ModRef%20Analyses.pdf page 15, which says that replacement with null ptr is safe. So I did two runs, one where we do pointer replacement only when they have same underlying objects, and the another where we also replace pointers with null.

In D143129#4098809, @nikic wrote:

This looks okay on the surface, but this needs at least a spot check of llvm-test-suite IR diffs to get an idea of how much this affects codegen. If there are no substantial regressions, let's go with it. Otherwise we may need to think harder about how we can mitigate regressions.

In D142420#4089859, @tschuett wrote:

Would it help to compile a lto.f90 to lto.o and check with llvm-objdump that there is a special section?

Add the new warning to a group to fix clang test failure.

• Address comments.
• Fix formatting for some old code as well.
• Update commit message.

In D142420#4081066, @kiranchandramohan wrote:

We probably need tests for the following:
-> Additional linker invocation options are created. (Is this the plugin option?)
-> Test to ensure that the emitted object code contains the additional info require for LTO.

I see some additional info in the LLVM IR generated with LTO in clang. Example below. Isn't this info required for Flang as well?

^0 = module: (path: "", hash: (0, 0, 0, 0, 0))
^1 = gv: (name: "add", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 1, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 8, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 0, returnDoesNotAlias: 0, noInline: 1, alwaysInline: 0, noUnwind: 1, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 0)))) ; guid = 2232412992676883508
^2 = flags: 8
^3 = blockcount: 1

In D142420#4080402, @kiranchandramohan wrote:

Thanks @mnadeem for this patch. Have you tried LTO and seen improvements? Or should flang emit additional info for this to work well?

This patch closely mimics clang behaviour with two exceptions:
No standalone option -flto for fc1. This is not needed if we always pass flto=xxx
Change the codegen option name from PrepareForLTO to PrepareForFullLTO to be more precise.

Is this a simplification/improvement or is there a strong case for these changes in Flang?

Are additional changes required for emitting assembly or bitcode?

ping

• Use fewer barriers and existing acquire/release instruction sequence.
• Add a shouldInsertTrailingFenceForAtomicStore() function.

In D141748#4060636, @efriedma wrote:

The explanation makes sense.

Can we make this specifically apply to _Interlocked* functions, instead of all atomics on Windows targets? I'd prefer not to impose this performance penalty on other users of atomics if we can avoid it.

In D141976#4061262, @sscalpone wrote:

@mnadeem Please update the documentation llvm-project/flang/docs/Directives.md to include dir$ loop count. Thank you!

• Address comments.
• Add fc1 tests.

In D137995#3931005, @kiranchandramohan wrote:

We might need -fc1 tests as well.

In D137995#3944145, @awarzynski wrote:

Thanks for implementing this!

Processes target cpu and features in the flang driver. Right now features are only added for AArch64 because I only did basic testing on AArch64 but it should generally work for others as well.

X86 is a very popular target and we have pre-commit CI as well. And X86 buildbots :) Please include X86.

Question: are the option semantics identical that what you get in clang -cc1? If yes, could you add a comment in the summary?