Is this compatible with dowstream targets defining their own intrinsics outside of Intrinics.td (in LLVM)?

In D143936#4124005, @gysit wrote:

We plan to add alias scope / access group attributes to the memcpy intrinsics soon. That will be good opportunity to add a test. I hope it is ok if I land tomorrow morning European time?

I wonder if we should add a test for this. Thanks a lot for the fix here.

This change broke our backend. LLVMIRIntrinsicGen.cpp::emitIntrinsic also allows for intrinsics to be included in alias scopes, but it now asserts at ModuleTranslation.cpp:1034 only supporting LoadOp|StoreOp.

Committed at 37c750a5fd13 (forgot to mark in the commit message).

Thanks for review.

Hi I've added reviewers from the git log. I wonder if this change is acceptable which would help us avoiding to downstream the entire pass. Thanks in advance

In D43876#1030514, @junbuml wrote:

I am open for suggestions. I think peeling off iterations at the end as Roman suggested might be worthwhile too. And opportunistically peeling off a few iterations, if the distance between start and end are low, but unrolling does not apply.

Peeling off iterations at the end might hit the case I'm trying to optimize. I will be happy to support it.

Ok to me for the convenience method but please get one more opinion

In D133603#3784824, @craig.topper wrote:

What about EVT::getTypeForEVT?

Added missed EVT::getTypeForEVT.

Re-opening

This patch is wrong, abandoning.

LGTM

In D129161#3663456, @barannikov88 wrote:

In D129161#3663186, @JamesNagurne wrote:

This was the issue I commented on in both of your comments re: including the back edge of the circuit in the Latency calculations for NodeSet. Our backend 'skips' the PHI and calculates the operand latency across the backedge to the true use.

I think I was talking about something different, most probably updatePhiDependences.
As you can see there, the Anti dependence created in this function gets the latency 1, not considering the latency of the instruction which feeds the PHI. Similarly, instructions which depend on PHIs get latency 0. This roughly means that the latency computed between the real instructions connected through a PHI node is 1, which in many cases far from accurate.

We recently resolved this by implementing a post-expansion insertion of scheduling barriers. Each modulo cycle is considered its own region and is, therefore, not reordered. There's some magic in the post-RA scheduler that undoes this so that we can actually schedule the whole kernel, but I digress.

Are you working on a backend where writing a test for this is feasible, like Hexagon?

1 -> 2, 1 -> 3, 2 -> 3, and 3 -> 1
Assume edges are all weighted 1.
Circuit 1 -> 3 correctly calculated as Latency = 1

Added more reviewers with history of the modulo expansion code, since I believe there is no feasible test for upstream targets, see commit message.

Reverted the patch in repo and re-pushing again. The only difference of the new push is s/enum_seq_inclusive/enum_seq/ in TargetLoweringBase.cpp.

Rebase only.

In D126386#3537423, @hgreving wrote:

There are a few lit failures, I will look at those first.

Updates patch. Let's do it straight forward and support cases where no branch is needed to the fallthrough.

There are a few lit failures, I will look at those first.

In D125574#3533949, @fhahn wrote:

In D125574#3532385, @Meinersbur wrote:

LGTM. Please wait for @fhahn LGTM as well.

It looks like the patch was landed but the commit message seems a bit out-of-sync with the code.it

In D125696#3532788, @aeubanks wrote:

precommitting a test for an approved patch generally shouldn't require review

Friendly ping on this review.

Friendly ping on this review.

In D125574#3516111, @hgreving wrote:

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Should be done.

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Rebased to https://reviews.llvm.org/D125696

Whoa another corner case.

Added a few clang-format on/off. I suggest to accept some of the enum changes in IntrinsicEmitter.cpp, Function.cpp, it feels unnecessary not to accept clang-format here. I wonder if these deltas are due to different clang-format versions in the past.

In D125247#3503920, @craig.topper wrote:

Please upload patches with full context using git diff -U999999

Full context update (excl clang-format off notations yet)

In D125247#3503918, @craig.topper wrote:

In D125247#3503854, @hgreving wrote:

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

We've been ignoring clang-format on these files. Maybe we should add // clang-format off markers

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

Abandoning, may try the integer route or check the sel DAG failures more closely.