NFC. Usage of Optional. Rebased onto committed D93709 .

In D93755#2486652, @arsenm wrote:

In D93755#2486281, @simoll wrote:

This is one of those patches that has the potential to linger on without review for 1Y+ .. let me know if there is anything i can do to get this reviewed, as it really helps with bringing good vector architecture (long SIMD) support to LLVM. Thanks.

Can I interest you in moving to GlobalISel instead of trying to teach the DAG about new types :)

Title typo: 'isntert'

NEC. Rebased. Use variable name Opcode instead of OC.

In D93766#2486284, @frasercrmck wrote:

LGTM. Had to double-check that things were #undef...fed at the end of the header.

Can we accept this patch? In the end, it is a strict improvement over the existing one.. and it is "just" a unittest..

This is one of those patches that has the potential to linger on without review for 1Y+ .. let me know if there is anything i can do to get this reviewed, as it really helps with bringing good vector architecture (long SIMD) support to LLVM. Thanks.

We want somebody that has reviewed VP patches before to review the VP bits of this (getters, really). @kaz7 takes care of the VE parts. Thanks!

This now depends on D93687 - the build_vector lowering causes introduces more instances of insert_vector_elt.

• Rebased onto main.
• insert_vector_elt tests trigger vbrd code path because lowering of build_vector to insert_vector_elt is gone (D93759).

|& -> 2>&1 |

getSimpleValueType()

NFC

• Hopefully attached signext to all args in tests this time
• Adressed comments

You might want to fix the typo in the title before committing this.

NFC

• Use bool4 in docs.
• Rebased,

NFC

• Use signext for arguments in tests.
• No more than 80 chars in VEInstrPatternVec.td file.

"Stuff" is unspecific. How about we go by "[VE] SJLJ isel" or something along those lines for the title?

• NFC. Rebased.

• Fixed custom lowering code for packed vectors and updated tests.
• Removed unrelated BUILD_VECTOR custom lowering to insert/extract elt.

NFC. Use OC variable name consistently.

There should be test for this. You could use:

FYI i have tested this against check-all with clang and all (including experimental) targets enabled.

In case you were wondering, the ISD helper functions are stapled to this VE patch to make a nice testable unit.

Done. Please base your patches on the upstream main branch in the future. That makes it much easier to commit patches with Arcanist.

NFC. Removed stray comment.

Thanks! Do you need me to commit this on your behalf?

Thx!

In D93455#2463506, @kpn wrote:

In D93455#2462358, @simoll wrote:

In D93455#2460765, @kpn wrote:

Do you need tests for the verifier part?

Probably. With this patch standalone, we can only do "catch-22 testing": all uses of the bundle are illegal. We can extend that once the VP floating-point intrinsics are upstream that actually support these bundles.

Would replacing some of the constrained intrinsics with operand bundles on other intrinsics have the same safety guarantee in the middle end? Currently the middle end doesn't know about the constrained intrinsics so it handles code conservatively.

Not easily. This has the same re-occurring issue as enabling predication on all instructions: if we allow the constrained bundle on regular fp intrinsics overnight, existing optimization code will ignore the bundles and potentially break code.

That's a non-starter. People are relying on the strictfp options and #pragmas to work correctly already. That's why I had to add to clang the ability to turn it off the command-line option on targets that don't support it yet. So regressing and making strictfp unsafe again is just not an option.

An elegant way out for this would be to introduce an FPMathIntrinsic helper class that only matches intrinsics in the default fp environment (that is without the bundle). All code that looks for a specific fp math function can then no longer just take the intrinsic id but needs to cast the intrinsic to that helper class first.
Btw, the FPMathIntrinsic helper also fits well into the generalized pattern matching framework (D92086).

If there's a way to replace the constrained intrinsics with other, existing intrinsics plus operand bundles _while_ keeping the safety guarantee that we currently have then that sounds good. But the safety _must_ be the default until we get to updating optimizations to understand strictfp.

That's why i am proposing to initially use these bundles only for the VP intrinsics and keep the constrained fp intrinsics in place. When we are sure that all optimizations use the proper abstractions, we can move to replace constrained fp intrinsics with the bundled version instead.

• This patch is based on D93534 .
• Added missing vp.fsub.
• Added constrained fp mapping tests (whether the constrained intrinsic actually exists and has as many metadata params as the VPIntrinsic accepts). This is in preparation of D93455 .

Added VPModuleComplete - test that all registered VP intrinsics are declared in the testing module.

Ping. Where do we stand with this patch, can we LGTM or are there comments?

In D93455#2460765, @kpn wrote:

Do you need tests for the verifier part?

Probably. With this patch standalone, we can only do "catch-22 testing": all uses of the bundle are illegal. We can extend that once the VP floating-point intrinsics are upstream that actually support these bundles.

• Fixed a failing tests (there is now two more op bundles)
• Rebased.

nfc, comment cleanup.

• NFC. Fixed spelling, typos.

Thanks for splitting this off!

LGTM with nits.

Just this one thing, else, LGTM.

LGTM. Can we upstream the script that generates the intrinsic declarations? We don't need to run it in the build but it should be upstream so people can re-generate and understand the intrinsics.

AFAIK all comments were addressed. Could you have another look at/LGTM this?

Thanks! One nit, else good to go.