In D149333#4325875, @JojoR wrote:

In D149333#4315068, @asb wrote:

Shouldn't there be some test changes along with this?

Apologies for the lack of a response.
Yes, the usecase will be in next patch, we split big clang intrinsic like other vendor extension as D147935

In D149333#4315068, @asb wrote:

Shouldn't there be some test changes along with this?

In D141346#4157530, @JojoR wrote:

Separated header file would be better, maybe thead_vector.h, and also extend the pragma syntax to #pragma clang riscv intrinsic thead-vector.
so the content of thead_vector.h only need header guard #pragma clang riscv intrinsic thead-vector.

@kito-cheng From RVV pragma implementation, the 'xtheadvdot' intrinsics could been filtered out according to command line.
Of course I could add thead_vector.h and include vector.h, but is it really necessary ?

Any comments or suggestions ?

Separated header file would be better, maybe thead_vector.h, and also extend the pragma syntax to #pragma clang riscv intrinsic thead-vector.
so the content of thead_vector.h only need header guard #pragma clang riscv intrinsic thead-vector.

@craig.topper @kito.cheng @asb @eopXD @jrtc27
Any other suggestions ?

I am confused about the failures of CI testing and checked that is AddressSanitizer-Unit of x86,
it looks there is no relation between AddressSanitizer and this patch,
could anyone give me some hints for the failures ? thanks :)

In D139386#3974689, @reames wrote:

I have not looked at the change at all (yet), but a couple of high level points.

1. Please link not to the repository, but a specific version of the PDF. We need to know exactly what version of the specification is being implemented. For instance, I was looking at https://github.com/T-head-Semi/thead-extension-spec/releases/download/2.2.0/xthead-2022-12-04-2.2.0.pdf. Please update RISCVUsage.rst to contain the same information. This will form the user focused documentation.

2. Can you clarify whether the extension is based on v1.0 or a prior vector specification revision? From the document, it looks like this might be based on v1.0. Getting an extension based off v1.0 into tree will be much easier than one based on v0.7, so being clear is in your interest here. Ideally, you'd revise the specification document to be explicit here as well.

3. Please split the patch. The first patch for review should add the MC (assembly) part only.

4. I very strongly encourage you to attend the riscv sync up call happening on Thursday of this week. This will be a likely topic of discussion.

@craig.topper Thanks for your help :)

LGTM also

@craig.topper @kito-cheng Any suggestion ?

In D136415#3877015, @eopXD wrote:

@JojoR May you please upload the LLVM IR with -O0 (so nothing is triggered) of what causes the assertion error to hit to here so I can try reduce the a test case out of it?

@eopXD I saw the patch D132443 only support eq/neq condterm, and I want to add gt/lt,
any suggestion for me ? or you have plan to implement that ?

In D134893#3855134, @eopXD wrote:

In D134893#3855128, @JojoR wrote:

I have a question about patch D132443, why do not you put optimization in pass "iv-users" ? I think it belongs to IV analyzation.

because that the pass "iv-users" do analyzation only without any transformation ?

IVUser is an analysis pass, and the LSR pass uses it before doing strength reduction.
This transformation is beneficial after LSR, not before it.

I have a question about patch D132443, why do not you put optimization in pass "iv-users" ? I think it belongs to IV analyzation.

There is a error report as below if i apply this patch on branch main 8d4ebd1a7c9e1f47a4a610aeb41d1613f822ee20
error: Terminating value is not safe to expand, need to add it to predicate.

@arichardson Hello ?

Any other suggestions ?

In D134168#3801800, @craig.topper wrote:

The description doesn't clearly describe the effect of the patch. My understanding from reading the user of this function is that the alignment of allocas and global variables used by memcpy are increased in CodeGenPrepare. This results in less memory operations. In the case of 32-bit dhrystone, it looks like we have an explicit call to the memcpy library function. I guess by aligning the pointers we allow the source and dest to both be word_size aligned so we can use the full word copy loop?

In my local testing for riscv64 this didn't seem to affect dhrystone performance.

In D134168#3801742, @jrtc27 wrote:

Set default preferred alignment for MemIntrinsic like memcpy according to arch32 or arch64,
it will improve performance.

What are arch32 and arch64?

e.g. dhrystone with "-O2" boosts performance by 50% on arch RV32.

On what implementation? Does this affect actually-useful benchmarks, not just dhrystone? I would assume so, but it'd be more useful to get numbers for meaningful benchmarks rather than ones people should've long since abandoned.

Thanks for your commit :)