Thanks for addressing the changes. LGTM but, as Mahesh mentioned, it's probably worth getting someone with experience in TOSA to take a look.

I can confirm that D125590 is not fixing the problem. D123901 broke our mlir-based project (IREE) due to an Operation * self-cast. Only with the proposed changes in this diff things get back to work.

Thanks for working on this, Rob! I really can't comment on the actual implementation from the TOSA perspective. However, I would suggest not enabling this by default. Architectures with full 64-bit support should do better with the 64-bit lowering. I can follow up with a patch to enable this for the RISC-V specific cases.

Thanks!

Thanks, Amy! In general, moving the select op before the operation makes more sense to me since the op might have side effects that would make the execution incorrect otherwise.
I hope we can replace all of this once we have the masking proposal in place.

Much better, thanks!

Thanks for the contribution, Javier! Some comments inline.

LGTM, thanks!

I'll commit it tomorrow if no more comments. Thanks!

Addressed feedback.

LGTM, thanks!

I'll let @bondhugula clarify but my impression about his comment in D120776 is that we should be able to have a single implementation that works for both ScfForOp and AffineForOp by using/extending interfaces like LoopLikeInterface? Maybe I got it wrong. In any case, adding a scf specific implementation is also great. Thanks!

Thanks for the feedback! I addressed the trivial one. The cleanup/generalization would need more thought before we move forward. More info below.
I'll move forward with this if no more comments.

Addressed feedback.

Thanks, Amy. LGTM once Uday's comments have been addressed.

Thanks, Amy! I think this is a very interesting extension! Some comments

Thanks, Uday. A couple of comments that would need attention. LGTM. You can move forward after addressing them.

Just curious if F32 data movement would not just work for any 32-bit entity, but change looks good to me.

An internal test exposed a bug in the implementation. This new patch
fixes the issue (see 'areDimsTransposedIn2DSlice') and improves documentation.
Could you please take a quick look?

Generalize the implementation even more by using shape_cast before and
after the transposition.

Thanks, Uday. LGTM!

Thanks, Aart!

Thanks, Aart! LGTM

Thanks, Sergei! Much clearer now. I think your proposal makes more sense to me now. The problem is very specific to the way you are generating the code but it's clearing exposing some corner cases of the create_mask operation. I'll leave it to Aart, though, since he has much more context on these ops (I'm not even sure I understand why we have the constant and non-constant variant of this op so I can't fully understand the implications of this change).

Thanks for the contribution, Sergei! I think I don't have enough experience with this op so I'll leave this to @nicolasvasilache.
How do you end up generating values that are out of the expected bounds of the mask?
Folding invalid values into valid ones could be surprising and maybe lead to silent bugs (?) but maybe it makes sense for this op. An alternative would be generating code in your use case to handle these out-of-bounds values and truncate them accordingly before they are passed to the create_mask op. I would useful if you could share a bit more about your use case.

LGTM. Just a minor comment.

Thanks, Rahul for contributing this! It's great that it can be integreated with the existing walk methods. LGTM. I'll leave the final approval to @rriddle.

LGTM. Just a comment about the test.

Addressed the feecback. The data layout spec is now passed using a string.
Much better. Thanks for the feedback!

In D112117#3185648, @reames wrote:

The change I'm working on is D115460. To my knowledge, there's no interaction with the vectorizer here, it's simply a code cleanup. e.g. Do the same thing for all types of poison generating flags. We may be too conservative in some places, but consistency has a strong value of it's own. At least per the tests being changed, the vectorizer is not impacted.

Thanks!

In D112117#3183418, @reames wrote:

JFYI, I've been taking a look at trying to get FMF flags into the generic dropPoisonGeneratingFlags. So far, the test diff (which looks a lot like this one for the same reasons) has exposed a bunch of cases where the existing code had problems. I'm working through fixing them individually, with the goal of getting a reasonable diff.

The most relevant one which got some discussion on this review is the reduction expansion. That appears to be entirely an artifact of code which a) does nothing without the FMF handling in the generic code, and b) serves no purpose even with it. 0d13f94c and b24db85 simply remove most of it, and fix up some comments.

Thanks for helping with the 0-d vector problem! Added some minor comments. I have a question about the lowering of 0-d vectors to the scalar world. See comments inline.

Thanks, Uday. LGTM but I'm not super familiar with the loop unrolling pass. I'll defer the approval to someone with more expertise in the pass.

LGTM. Just one minor question. Thanks!

Dropping FMF doesn't seem to be needed after all. See D111846

Dropping FMF doesn't seem to be needed after all. See D111846

Thanks for all the feedback! I think this will cover a good part of the cases that could actually go wrong in practice. We can accommodate more cases as we see fit.

• Maybe it's worth adding this link to the commit message: https://groups.google.com/g/llvm-dev/c/P_CXf5hPro8
• Typo? distinguishes between and intrinsics and an inline_asm implementation.

Thanks for addressing the feedback. LGTM. Just one minor comment. Feel free to fix it a commit it directly.

LGTM, thanks!

I talked to Nuno in private and he mentioned that I could go ahead and commit the changes and address any minor feedback in a separate commit since he is very busy right now.
I'll commit this on Monday if no other comments.

Thanks a lot for the fix! LGTM. Just a minor comment.

Thanks, Florian! I addressed the feedback.

• Updated and added more comments.
• Simplified redundant condition.

In D112808#3129562, @arnab-oss wrote:

ping @springerm and @dcaballe , can you please let me know whether the revision is ok or if any more changes are required to be made? Thanks in advance!

Any other comments? :)

Since it was already implemented in a previous version of the code, I restored the logic to drop flags
from Widen and WidenGEP recipes following Florian's suggestion. These changes only impact Florian's
test case. I modified the test case to also have a vector GEP feeding a masked load to also cover
that use case.

Added new test. Thanks, Florian!

Fixed naming convention issues

Thanks, Nicolas! LGTM.

Ok, I'll wait for Florian's approval to land this patch.

Fixed failing test

Thanks a lot for the contribution! LGTM. Just some minor comments.