Actually make sure this builds. :)

In D93981#2476868, @mehdi_amini wrote:

For Wpessimizing-move : is this a clang or gcc warning? I remember that they would give opposite interpretation in this case.

I did this under the assumption that it is better to avoid -allow-unregistered-dialect if possible. However, I'm happy to drop this patch if that's not true.

Fix commit message

In D89324#2328592, @mehdi_amini wrote:

The semantics of async or not is *very* different from most analysis point of view. It isn't clear to me that having a single op for modeling both instead of a new operation is the right tradeoff here: I'm afraid it is too error prone.

In D81156#2088659, @MaskRay wrote:

In D81156#2088609, @sanjoy.google wrote:

In D81156#2084324, @jhenderson wrote:

In D81156#2084273, @sammccall wrote:

In D81156#2084209, @jhenderson wrote:

In D81156#2084176, @sammccall wrote:

This is surprising and unsafe behaviour. Can we reconsider changing the default, at least until a more careful audit of the places errs() is used?
In particular, if errs is used from multiple threads (with a lock) and outs is used from a single thread (so no lock needed) then this introduces racy flushes to outs() concurrent with writes.

Thanks for highlighting this, it's been brought up on D81525 as well, so rather than discuss it in two places, we'll look at it there, if that's okay?

Happy to discuss there. It's a separate issue though, the discussion there is about initialization being racy,

Yeah, I realised that after posting that previous post, sorry! Still, probably best to keep the discussion in one place for now.

Did we resolve this? In an internal XLA build tsan complains about this being racy, and I'm not sure if the problem is with how we are using llvm::dbgs() or with this patch. If the latter, please rollback while we figure out a non-racy way to achieve the same thing.

Does 030897523d43e3296f69d25a71a140d9e5793c6a fix the race?

In D81156#2084324, @jhenderson wrote:

In D81156#2084273, @sammccall wrote:

In D81156#2084209, @jhenderson wrote:

In D81156#2084176, @sammccall wrote:

This is surprising and unsafe behaviour. Can we reconsider changing the default, at least until a more careful audit of the places errs() is used?
In particular, if errs is used from multiple threads (with a lock) and outs is used from a single thread (so no lock needed) then this introduces racy flushes to outs() concurrent with writes.

Thanks for highlighting this, it's been brought up on D81525 as well, so rather than discuss it in two places, we'll look at it there, if that's okay?

Happy to discuss there. It's a separate issue though, the discussion there is about initialization being racy,

Yeah, I realised that after posting that previous post, sorry! Still, probably best to keep the discussion in one place for now.

In D74185#1865186, @ABataev wrote:

In D74185#1865175, @sanjoy.google wrote:

In D74185#1865088, @ABataev wrote:

Thanks, will check this.

Should we revert the commit while you investigate?

The problem here that this patch does not introduce new vectorization, instead it just triggers the existing vetorization for more cases. If there is a bug in the vectorizer, this patch just allows to reveal it, not introduces it.

In D74185#1865088, @ABataev wrote:

Thanks, will check this.

My understanding is that both the unsigned and signed ranges are valid and we are free to use either one, so using the unsigned one is more beneficial in an unsigned context.

This needs a specific test case.

In D71563#1786529, @SjoerdMeijer wrote:

Thanks for taking a look!

llvm.loop.decrement.reg is not documented in the langref. Is this a target specific thing?

No, it's a target independent thing, and is generated by the generic Hardwareloop pass. It is defined here:

https://github.com/llvm/llvm-project/blob/ff07fc66d9eef577f3b44716f72e581a18cd9ac9/llvm/include/llvm/IR/Intrinsics.td#L1304

Also, is the entire gamut of optimizations this enables OK? For instance, would it be okay to LFTR replace the trip count of the loop such that it doesn't use the loop_decrement_reg intrinsic anymore?

Hmmm, I don't have a good answer for that yet. First, just to be clear, the llvm.loop.decrement.reg intrinsic is exactly the same as a sub expression, so I think SCEV analysis functions should be okay. But yes, I think I can imagine that SCEV transformations that modify the loop update expression could potentially be problematic. But this is a bit of unexplored territory for me, so first I need to look into what LFTR exactly does. So, I will need to so some homework first, but if we assume for a moment that there will be problematic cases with SCEV transformation modifying the loop update expression (if they would ignore the loop.decremenent intrinsic if there is one present), does that make this approach completely unfeasible, or would you still see possibilities to use the SCEV analysis functions?

llvm.loop.decrement.reg is not documented in the langref. Is this a target specific thing?

lgtm

lgtm

If the SCEV expression is mathematically correct I think this is a problem with SCEV expander. If it is expected to preserve LCSSA it should add the necessary PHIs.

Last time I looked at this I concluded it would be difficult to make enable something like this consistently, but no harm in trying.

lgtm

lgtm

lgtm

lgtm with some nits