Please discuss this commit at https://discourse.llvm.org/t/inlining-mathematical-function/68042/1

This patch is for reference only.

LGTM. Thanks.

@caojoshua Thanks for the fix. Almost looks good to me. I added some minor comments.

Ping.
I addressed @dmgreen's comment.

Ping.
I addressed @dmgreen's comment.

Some tests are added.

Test for FP16 is added.

As per @dmgreen's comment, update the source comment.

As per @dmgreen's comment, change -mcpu=cortex-a710 to -mcpu=neoverse-n2.

Abandon. I'll post splitted (and improved) patches.

Addressed to @zahiraam's comment

ping

In D132828#3941665, @seanxilinx wrote:

@kawashima-fj, I think it is a good optimization pass. I want to integrate it into my product. Is there any license issue since it is not taken by LLVM? Thanks

In D138088#3937680, @aaron.ballman wrote:

Thank you for this cleanup! In general, I thin this looks correct. However, I know we've had to fix a bunch of options that cause the sphinx build to fail (IIRC, oftentimes due to duplicate options) and our precommit CI doesn't test the documentation build. Did you try building the docs locally to ensure there are no new warnings/errors from Sphinx?

In D138117#3933644, @zahiraam wrote:

What I meant was that the changes about the semantic mode approx-func that you made here https://reviews.llvm.org/D138109 , should have included the changes that you are making in this patch about approx-func.
In general, the changes to the users' manual should be in the same patch than the code changes.

Update a test to include the ffast-math -ffp-model=fast pattern.

In D138117#3931109, @zahiraam wrote:

It would probably be better to make these changes with the appropriate code changes you are making for each semantic mode. And may be keep this patch only for the other changes you are proposing.

I don't know it is inteded that -fno-fast-math does not imply -ftrapping-math and -frounding-math.
-ffast-math and -fno-fast-math is not symmetric in RenderFloatingPointOptions.

In D138107#3930322, @dmgreen wrote:

I mean a few different patches. It can be good to commit things in smaller chunks in case some part of it needs to be reverted. The patch looks sensible from what I can tell, I just feel it is doing a little too much all at once.

Is your patch based on my D138107? If so, please make a patch based on the latest main branch and commit/push this patch first. D138107 will take time to land.

In D138107#3930239, @dmgreen wrote:

I think it would be best to split this up into some instruction groups. The scalar were already being handled by https://reviews.llvm.org/D134260 (ANDS can also be added). I hadn't committed that because it ran into the same pcsections test failure. I have a patch to fix it up though, I can put that up for review and get D134260 committed.

The others could be split into scalar fp/neon/sve/etc.

I found some errors in Clang Compiler User’s Manual. I will update of the Manual in another patch.

@melver
As shown in the change of llvm/test/CodeGen/AArch64/GlobalISel/arm64-pcsections.ll, the Machine InstCombiner pass removes pcsections metadata when replacing MachineInstr. I'm not sure it should nor not.

In D132828#3757973, @kawashima-fj wrote:

In D132828#3757715, @fhahn wrote:

Have you considered implementing this as MachineFunctionPass instead of an LLVM IR pass?

No. Just because the original patch D67383 was implemented as an LLVM IR pass and I'm not familier with MachineFunctionPass.

Doing the transformation on MachineIR would allow for more precise cost estimates, including more accurate information about register usage, selected instructions and processor resource usage. MachineCombiner.cpp might be interesting example to look at for similar (although simpler) transformations with relatively accurate uarch-driven cost-modeling.

Thanks for your advice. It seems to have an advantage but porting to MachineFunctionPass may take time for me. I'll see about it.

In D132828#3757715, @fhahn wrote:

Have you considered implementing this as MachineFunctionPass instead of an LLVM IR pass?

A revised patch is posted as D132828. Please review it.

LGTM. I talked with @ytmukai and he explained to me how he updated the info. Though I didn't review all the updated lines, his approach is reasonable.

@c-rhodes This patch includes a follow-up to your comment D128631#3618052 . @ytmukai is my colleague. Most other changes are refinements.

Thanks @congzhe . LGTM.

Thanks @congzhe . Though I'm not familiar with DA, it looks good to me. I'll leave the detailed review to the experts.

Pushed to the main branch. I'm sorry, I forgot to add Differential Revision: line. The commit is d9a9c992d190dd6645ea911b66cf0cadba0dadc3 .

@Meinersbur Thanks for your review. I've updated the patch. Correction of LLVM_DEBUG message and adding test patterns.

@Meinersbur I'm sorry for being late. I updated the patch. Please review again.

@fhahn Ping. I addressed your comment.

@fhahn Ping. Could you confirm?

@Meinersbur Thanks for your review. I'll update the patch.

@fhahn Ping. Could you confirm?

Ping.
@fhahn @asbirlea Is this OK?

In D75594#1927988, @ikitayama wrote:

In D75594#1927959, @kawashima-fj wrote:

Yes, https://github.com/fujitsu/A64FX contains the official microarchitecture information of A64FX. I wanted to include the URL in the Git commit message but the disclosure was not ready for it at the time.

Can you do it at the next commit opportunity as this reference manual should be broadly read by the Arm developer community?

I'm sorry for the late response. I (and my colleague) updated the test.

Update test.

In D93503#2479425, @fhahn wrote:

This seems reasonable to me, as we should only reduce the number of loads executed during PRE. If they are in the same loop, the metadata should stay valid, as we do not access any additional locations.

Would it be possible to add a simple test case where we move a load from a sub-loop into a parent loop?

Simplify the test code.

Thanks for reviews! I'll commit this next week unless other new comment is posted.

Ping.
Can anyone review? Is there an appropriate reviewer?

In D93791#2471417, @tschuett wrote:

Is there a way to specify the maximum or even the set of supported SIMD-widths for SVE cores?

Remove WriteID512 and update comment.

Ping.
I know many people are on holiday this week. If you have time, please review this patch. I'm on holiday from tomorrow.