Better this way, let's keep the changes contained to one per patch. If there's interest to change that behaviour or not, it should not hold this patch, which looks good to me now. Thanks!

In D62997#1591113, @psamolysov wrote:

@hsaito The test no_switch.ll failed because remarks aren't written now by default, so without -pass-remarks-analysis. I've just removed the corresponding check from the test. But, I think, the NFC label from diff/commit should also be removed in this case because the behavior is changed a little bit. No other problems with tests in the LoopVectorize directory. If this diff is applied, the entire set of tests passes.

As far as I can see this is an NFC refactoring with obvious benefits to readability and extensibility. LGTM too. :)

In D53613#1566137, @lebedev.ri wrote:

It's best to submit a new review.
Else no one sees the description as a mail.

Adding llvm-commits for wider audience

lgtm, thanks!

In D62997#1551946, @psamolysov wrote:

The diff has been updated: private methods were removed, the function 'reportVectorizationFailure' was moved to LoopVectorize.cpp and declared in LoopVectorize.h (namespace llvm). I've removed the passName parameter and pass LV_NAME as pass name to ORE but this change breaks 3 regression tests:

I agree with Hideki, this could be in LoopVectorize.h.

Sorry Pavel, tough week. it's at the top of my list.

@huntergr do you have an account on bugzilla? I couldn't CC you on that bug.

Committed as r362736

Much cleaner, thanks! I think we should deal with extending the functionality in a separate patch, since that's already a good localized improvement.

In D62478#1518679, @psamolysov wrote:

I don't understand why having a function with a lot of arguments is better than a private method getting access to the class' members. Why are private methods in C++ at all, they also are used only from class methods and can be replaced with static util functions getting required class members as arguments.

In D62478#1518814, @psamolysov wrote:

I believe the LoopVectorize class also should use this function since there are a number of occurrences of the same pattern

In D62478#1518099, @psamolysov wrote:

@rengolin It is because the function uses OptimizationRemarkEmitter *ORE, a member of the LoopVectorizationLegality class.

Looks nice, thanks! Though, does it need to be a member function? Why not just a local static function?

In D62110#1511129, @nickdesaulniers wrote:

LoopVectorizationCostModel::expectedCost default initializes its return value, an instance of LoopVectorizationCostModel::VectorizationCostTy declared as:

using VectorizationCostTy = std::pair<unsigned, bool>;

so looks like it's possible.

Can expectedCost ever return zero?

looks obviously good to me. thanks!

In D32530#1477041, @hfinkel wrote:

Exactly. Non-constant values can become constant. Constant values can be guarded by vscale-dependent runtime guards (both hand-written and compiler generated). My preference is to leave this not restricted to vscale == 1 values, but rather allow all values that can be supported at runtime, and have it be UB if, at runtime, the relevant index is not available.

In D32530#1466746, @huntergr wrote:

So if we wanted to keep them as intrinsics for now, I think we have one of three options:

1. Leave discussion on more complicated shuffles until later, and only use scalable autovectorization on loops which don't need anything more than splats.

In D32530#1464722, @hsaito wrote:

If we are going for more than element-wise Instructions, we need to have well defined and agreed semantics for each of those, and that should be part of the LangRef for each such Instruction.

In D32530#1456113, @huntergr wrote:

• Clarified that the runtime multiple is constant across all scalable vector types, even if the constant value isn't known at compile time.
• Removed extra whitespace.

I'm curious as to how will we generate these flags.

In D59335#1428404, @sanjoy wrote:

langref part seems to be missing.

I'll add it if/once reviewers are okay with this approach.

In D59149#1427946, @hsaito wrote:

Cost model for emulated masked load/store is completely broken.

comment is still valid. What would it take to address this properly?

Each target to run many applications/benchmarks to come up with the "right" adjustment to the cost model. No way around that.

The concept of "hacked" is lost when you move up to TTI. I'd change the logic to reflect that this is making it "prohibitively expensive" instead of "hacked value".

I'm not against moving docs there but I think there should be a conversation about the broader subject of docs, github, wiki, etc.

In D53927#1412486, @joel_k_jones wrote:

I'm trying to understand what log10 behavior got changed. Could someone out where that change is?

In D53927#1412003, @joelkevinjones wrote:

I'm not sure what the proposed split would consist of.

Hi @steleman, that wasn't a "loaded question", it was an honest one. Let me try to clarify what that means for all parties.

@t.p.northover Is this the correct behaviour for Apple's syntax?

Sorry for the delay, this fell out of my radar and just saw the ping now.

Perfect, thanks!

@bryanpkc debug build makes sense, thanks! @steleman, I'd rather just order now and try to rector later. This patch has changed enough tunes already. :-)

In D53927#1399306, @steleman wrote:

Just did a rebuild after moving lgamma after isoc99. No warnings, no errors, and no difference in my test results ...

Thanks Stephan, now looks good.

Thanks for your answers, they make sense. Other than the mistake I made on disabling exp calls (you were right), looks fine.

Thanks, LGTM!

In D53927#1397870, @steleman wrote:

Because __tgamma_r_finite doesn't exist, at least on Linux. :-)

See /usr/include/bits/math-finite.h.

In D53927#1397785, @shibatch wrote:

tgamma stands for "true gamma", and it is the function name specified in the ANSI C standard.

Funny thing is, SVML is also only supported, AFAIK, for Intel. I agree that we should emit errors, but we should also emit a similar error on SVML.

I have a few comments inline, mostly harmless, as the change is almost entirely mechanical and makes sense.

@hfinkel There are changes in this patch, as well as its LLVM counterpart D53927. Please, re-review.

Right, this is a completely different patch and it seems that the original proposal hadn't been tested thoroughly.

Since the previous version was approved already, I'm "requesting changes" so that we can look at it again, together with D53927 to make sure it's still the right way to go.

Adding llvm-commits for wider audience

dword is indeed widely used. I'm surprised we didn't support that yet. Apparently, neither does Arm, only X86, Mips and RiscV.

In D53695#1286251, @huntergr wrote:

I didn't move it to the proposals directory, since I'm not sure if we would lose the inline comments that way.

In D30247#1375520, @Ayal wrote:

BTW, targets that have efficient masked operations may also find it useful for cycles and size to vectorize (including epilogs) under -Os, and following r345705 possibly also with enable-masked-interleaved-mem-accesses turned on.

LGTM, thanks!

In D53355#1366988, @npanchen wrote:

Sorry for this long reply. As Satish has pointed out in his review, this VPPHINode will be used to generate blending.

Sounds good. Please add simple tests for +inf and -inf on both double and float. Perhaps also float to double and vice versa from the IR constant (like the changed test shows), too. Thanks!

Hal approved the Clang side, so this one looks good, too.

Perhaps best to ping Clang's commit, as that's the review that is blocking this one.

Thanks! I've updated the reviewers on the Clang review (D53928).

Adding clang/omp developers for proper review. Please feel free to add more.

If you git log cmake/config.guess you'll see that all the past changes either remove functionality or add obvious parts that shouldn't inflict on copyright.

Right, that's all very hypothetical, but the change is harmless otherwise and generally an improvement of the code. I can't see any impact this would have in code generation or compilation time, so LGTM.

Getting a bit late in this discussion, as we had an internal one just recently.

Right, I'm happy with the patch and will let @fhahn do the final review and approval. Thanks!

In D53927#1328003, @shibatch wrote:

Where, when, how and by whom are these #pragmas inserted? Are they silently inserted by clang? Do they have to be manually inserted in existing code?
What is the effect of these #pragmas? The ARM closed-source commercial compiler requires OpenMP. How will these #pragmas be implemented in the open-source clang? Will they import OpenMP interfaces in the open-source clang as well?
If importing OpenMP interfaces is not triggered by the insertion of these #pragmas, why is the argument to -fveclib called 'openmp'? I would find it baffling to have to type -fveclib=openmp on compile-line, but ending up not using OpenMP.

I think you should post these questions as a reply to the RFC.

As a starting point, this sure looks good to me. Thanks!

In D54412#1327560, @fpetrogalli wrote:

Is there a document that explains how to submit an RFC?

In D53927#1327204, @steleman wrote:

Then they are in trouble with a hypothetical distro SLEEF.

That's because no commercial distro - if they ever decide to include SLEEF - will compile SLEEF for the maximum CPU feature-set possible. They do the exact opposite - they compile it for the lowest possible common denominator.

For example, on AArch64, you won't get distro libraries compiled specifically for -mcpu=thunderx2t99. Or on x86_64 you'll never get -mavx512ifma, for example.

In D53927#1327020, @steleman wrote:

I am taking it on faith that SLEEF won't keep changing their mangling and ABI.
AFAICS, SLEEF isn't included in any Linux distro. I can't find any reference to it in RH/CentOS/Fedora, or at rpmfind. Or Ubuntu or SuSE.

In D53927#1326908, @shibatch wrote:

Today, we don't have SVE, so we don't generate SVE bindings, all fine. Say, next year you upstream SVE bindings in SLEEF and then change LLVM to emit those too.

SLEEF already has support for SVE.

Overall, this change looks ok to me. The tests look good, too.

In D55445#1323547, @mgorny wrote:

One would hope someone would have done similar work on a compatible license, after all these years...

I don't think that's likely to happen, given the very narrow use of this script and how complex it logic is. I think it would take tremendous effort to reinvent all those hacks from scratch, and it would be hard to prove the work was done independently without violating copyright.

Adding more folks due to the license issue.

Right, that one is a hairy test, and I can see how this would break that test if not right. :)

That's very useful, thanks! LGTM.

Nice clean solution, thanks!