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!

Accepting as this seems to fix the fast-math issue that was the reason why we reverted. Let me know if this still breaks things.

This makes sense to me, but I'm not well versed in LLDB testing. Adding Pavel and Omair to approve it.

LGTM, do we have any tests for llvm-mc output?

I'm not sure what's the supposed behaviour on Apple syntax, but the tests show both apple and eabi syntax changing to the recommended encoding, so this LGTM.

@Carrot, can you check if this fixes the fast math issues on your side, please?

Perfect, LGTM. Thanks!

So, IIUC, the way you only get the interleave info on instructions and map to VPlan is because we don't yet have scalar evolution in VPlan, so we need to do that in Instruction and then map to VPInstruction.

Just had a look again and it's looking great, thanks Florian!

In D54378#1294266, @chandlerc wrote:

I really do find the break with convention here somewhat deeply unfortunate. It makes parsing and recognizing triples reliably much harder IMO. This really should be 'hurd-gnu' or 'mach-gnu' (or however you want to spell the 'OS' here).

In D54378#1294264, @kristina wrote:

That's not to say I'm against adding support for GNU Hurd, I'm moreso implying that regardless of what GNU Hurd guidelines say, if they may cause problems in LLVM ecosystem, especially as far as core targets (or rather platforms) go, you have to consider the large ecosystems that use Clang and LLVM and sometimes adapt to avoid causing issues downstream (for example with distro vendors with Glibc/GNU Linux being the core of main variations of server and desktop Linux distros).

In D54378#1294257, @sthibaul wrote:

To be honest, I hadn't even imagined that the patch could have consequences on non-Hurd systems.

In D54378#1294254, @sthibaul wrote:

Is that really supposed to work? I thought the rules was that the cpu parts and the vendor parts mustn't contain '-', and thus the splitting is never ambiguous, and the os part can contain '-' (heavily used by GNU/Linux, GNU/kFreeBSD, etc.)

In D54378#1294253, @sthibaul wrote:

Well, it definitely is for GNU/Hurd, now that very basic software pieces such as librsvg start _depending_ on rust, which is available only through llvm.

In D54378#1294220, @kristina wrote:

Yes but my concern comes from possible confusion with Glibc based Linux targets which are one of the "main" targets supported by LLVM. Either adding hurd or mach somewhere would be sufficient to disambiguate it from others, and yes I'm aware that they have a somewhat free format but they convey some sort of information, ie. with Linux targets gnu implies GNU/Glibc-based userland, so going by that convention one could easily assume that said triple simply implies GNU/Glibc/ELF userland. I think simply using gnu is too ambiguous, especially considering this is being added as an experimental target.

In D54378#1294221, @sthibaul wrote:

I understand your concerns, but that's really not something that we can change at this point, there is far too much software which has written i386-pc-gnu in their source code for us to change all that.

In D54378#1294215, @kristina wrote:

I'm not sure how GCC does it but this seems pretty out of line in comparison to all other targets, GNU makes sense to indicate that the target uses a GNU (usually with Glibc) userland but there's no indication of the kernel, which would be something like mk or mach. In which case I think i386-pc-mach-gnu or something along the lines would make sense, unless this is absolutely needed for compatibility reasons?

In D54378#1294204, @sthibaul wrote:

(I can not commit this myself)

In D54378#1294199, @sthibaul wrote:

The GNU/Hurd triple is target-gnu, simply.

I had no idea Hurd was still alive, but hey!

This looks ok to me, but I'd rather more people look at it before approving.

Right, I don't see anything wrong with it, but I'm adding other back-end maintainers and Jakob, TableGen's maintainer, so we're sure this is as harmless as I seem to think it is. :)

Hi Simon,

Reverted in r345465. Will take a look and land again when fixed. Thanks!

In D49168#1277837, @Carrot wrote:

This patch doesn't correctly handle isFast(). By default isRecurrenceInstr() should check I->isFast(), but for this pattern I is Select, isFast() doesn't apply to it, it should be checked against FAdd/FMul inside isConditionalRdxPattern().

This is looking good to me, thanks!

I also have a list somewhere, that I will add once I find it.

Hi Nikolay,

(very gentle ping) :)

Consistent messages.

Oh, you've done that. The diff isn't clear.

The failure in Arm had nothing to do with not being ELF, but the asm syntax was different.

I'm wondering... should we choose between dot and png? Or should we always print the dot file and, upon --png flag, also the png file?

Thanks Florian, your regex version is still easy to use and much cleaner.

Hi Florian,

Seems trivial. instructionsWithoutDebug is just an iterator filter and there are no early continues in the body of the outer loop, so this actually saves time.

Good catch! LGTM, thanks!

Could also be an architectural flag, giving this is a particular behaviour from Exynos. Then a simple splitsJumpTables() or whatever check would be enough.

In D32564#1258763, @t.p.northover wrote:

As you'd expect, size is pretty good. Over the test-suite (including externals) nothing regresses. The total benefit over all the code is 0.5%, with notable highlights of 25% in 401.bzip2, 7% in 403.gcc, and 4% in 177.mesa.

Changes to comment:

• Improved comments on isConditionalRdxPattern
• Removed instcombine pass from test
• Added write negative test
• Fix typo in CHECK line

In D49168#1258185, @hsaito wrote:

LGTM. Please wait for a few days to give others time to respond if they'd like to.

Hi Hideki,

In D52829#1255435, @paquette wrote:

OTOH, having a function flag or something like @rengolin suggested is probably a bit cleaner in the long term. I slightly prefer this position since it seems like it should be the verifier's responsibility to understand what to do with an outlined function. Also, if it does turn out that there are other weird cases, it'd be kind of overkill to add special outlining instructions everywhere just to appease the verifier.

Are there no other ways to identify an outliner function, say a function flag or a needed pattern, so we can change the verifier instead, and not fail when called-saved regs are used?

Hi Peter,

In D52149#1250772, @kristina wrote:

There's no intent to revert

In D52149#1250735, @kristina wrote:

I think it's okay to land this for convenience for now (once the issues are addressed) and if the Clang differential does not get approved revert this, which is not especially difficult since the support lib is quite isolated.

Adding more people that work around triples...

This is a slight duplication of what we already have in ARMSubtarget.h, and given that this is *only* relevant for AArch32, I see no point in this being in the Triple.

The other two reviews were approved. This one can now be closed. Thanks!

Thanks Peter. LGTM!

Thanks Peter! LGTM.

Corrections are always welcome. Please submit an update to this thread (or a new patch) to fix the changes you propose.

If j-s doesn't exists (remember, most of that list came from the ancient days of llvm, so could very easily be completely wrong, but "works"), I all for removing it and making a new (correct) CPU name as the default for armv6k.

In D18086#1043635, @falstaff84 wrote:

I seem to have a related issue: I am using -march=armv6k and -no-integrated-as, this generates the following output:
/tmp/empty-bc2ea3.s:4: Error: unknown cpu `arm1176j-s'

Thanks Florian! LGTM too.

Very detailed, using the modern infrastructure and helpful even to those not using clang or wanting to run external benchmarks.

In D49563#1168689, @evandro wrote:

I shall, at least for Exynos, but I'd like to hear from our friends at ARM too.