Incorporated review comments for D127910:

Fix some bugs in the range checks of immediate operands.

Moved test case acle_target_sme.c into clang/test/Sema/aarch64-sme-intrinsics/.

@sdesmalen @rsandifo-arm @aaron.ballman @erichkeane What is the plan for this patch? Is it going to be abandoned in favor of D139028, or can it be landed in its current form after addressing all outstanding review comments?

Removed EltTypeBool128 as suggested.

Addressed review comments.

Fixed minor bugs in the previous upload.

Addressed review comments.

Rebased on trunk and addressed review comments.

Rebased and cleaned up the patch. Also added _m suffix to the intrinsics as required by the amendment in https://github.com/ARM-software/acle/pull/218.

Added _m suffix to the intrinsics as required by the amendment in https://github.com/ARM-software/acle/pull/218.

Rebased and cleaned up the patch.

Rebased and cleaned up the patch.

Update patch with more context.

Rebased and cleaned up the patch.

Update patch with more context.

Minor clean-up. Sorry for the noise.

Updated the patch to define the __ARM_FEATURE_SME macro when the feature is enabled.

LGTM

Thanks for the patch. I just have some simple suggestions.

Removed incorrect IsPreservesZA flag from SVWRITE intrinsics.

Re-upload to add diff context. No change to the patch itself.

Rebased on the new D127910.

Removed some more unnecessary lines.

Removed some diffs that weren't necessary.

@david-arm, I have moved all the SME definitions into a new file, arm_sme.td. I have moved the common definitions into another file, arm_sve_sme_incl.td, which will be included by both arm_sve.td and arm_sme.td. SveEmitter has been updated to incorporate your suggestions.

Updated the patch according to review comments which suggest to treat SME as a different architecture.

In D127910#3885699, @david-arm wrote:

Hi @sagarkulkarni19, just a gentle ping to see if you are still planning to do more work on this patch?

LGTM.

The C++ standard says all the optimizations we want to do on new/delete expressions are legal whether or not the user replaces the operators . See [expr.new] (http://eel.is/c++draft/expr.new#12) . The weird issues this patch is trying to fix only show up when the definitions are available.

Just saying "any function with a definition isn't a target library function" probably produces a sane result in most cases.

Wasn't there some command-line argument to specify that a certain builtin isn't a builtin?

@bryanpkc you make a good point and it would be interested to see those patches!

For this patch I think that unless the changes you're suggesting are trivial, it would make sense to have any changes that are not required for the assembler as follow-up patches. I'm a bit cautious about this otherwise holding up SME asm support into LLVM 13, since those changes aren't necessarily required for the assembler.

Hi @bryanpkc, thanks for the comments. Do you also have an implementation for this?

Hi @c-rhodes, we had done some design work for the ZERO instruction, and it is interesting to see your implementation. I have some questions about the code, based on my understanding of the ISA.

LGTM. Thanks.

@tstellar Can this patch be cherry-picked into release/12.x? It fixes libomptarget testing in downstream compilers.

I agree with @richard.barton.arm that we could add -std= now, even if only for a subset of possible language standards. I expect that standard conformance checks would be quite pervasive, and it would be better to design the infrastructure for such checks earlier rather than try to retrofit them at a later stage when we have to start supporting f202x, etc.

@awarzynski, @sscalpone added the rule in C++Style.md in fdb58597e859. Perhaps Steve could explain the motivation for deviating from Clang's style guide (https://llvm.org/docs/CodingStandards.html#error-and-warning-messages)? IMO Flang should behave more consistently with Clang, especially if we start sharing more code between the two frontends going forward.

@erikjv Are you still working on this? Or is a better alternative being pursued? IIUC, this functionality is still missing in trunk.

Apologies for the bug and thanks @hliao for the quick fix.

Thanks Sanjay.

Pinging reviewers.

Removed the change to the default value of EarlyCSEDebugHash as per @lebedev.ri's suggestion.

Fixed linter warning.

In D86843#2246700, @lebedev.ri wrote:

In D86843#2246699, @bryanpkc wrote:

In D86843#2246545, @lebedev.ri wrote:

1. As a preparatory patch, we should adjust every test that uses EarlyCSE to also specify -earlycse-debug-hash, at least to those where it doesn't result in an assertion

Thanks for your suggestions, Roman. I have added the reproducer to the EarlyCSE tests, next to other similar test cases. Regarding your first suggestion, do you mean that we should enable -earlycse-debug-hash in all tests under llvm/test/Transforms/EarlyCSE/, or even all tests under llvm/test/ that may implicitly use EarlyCSE?

At least for every test that explicitly uses -early-cse.

That seems like overkill.

OTOH these bugs aren't fun, and they should be trivially caught by existing tests, since the test coverage should be good anyways.

In D86843#2246545, @lebedev.ri wrote:

1. As a preparatory patch, we should adjust every test that uses EarlyCSE to also specify -earlycse-debug-hash, at least to those where it doesn't result in an assertion

Added a regression test using the -earlycse-debug-hash option.

Cleaned up unused code.

Moved the fix into matchSelectWithOptionalNotCond instead of duplicating code.

In D80801#2102291, @efriedma wrote:

We are only pulling the trailing addition in with an existing multiply.

The problem here is that it's the "wrong" multiply: you have, essentially (A*B+D*E)+F, and you're turning it into A*B+(D*E+F). I don't think contraction is supposed to cover that.

Sorry for the late question, but I don't understand why this kind of folding is not considered reassociation. I thought reassociation was not allowed even when -ffp-contract=fast.

In D77603#1978909, @AndreyChurbanov wrote:

Yes, ideally barriers of all types should have these extra synchronizations, though with lesser priority I think. Because other barriers do not work by default, they need to be explicitly requested.

In D77603#1977658, @AndreyChurbanov wrote:

Sorry, found one more issue.

I think the first MB should be moved inside the block:

for (level...
  if (((tid...

Pair the memory barriers correctly at the same nesting level within the loop.

Add a paired memory barrier to the child thread's path after it finishes writing to its own data and before releasing the parent thread.