Thank you for fixing this!

Thanks for adding the asserts!

Thanks for your work! It looks good to me and I only have a few minor comments.

It is a very nice PR, thanks for the great work! I finish one round of reading the code and haven't read the MLIR tests yet. Would like to send out my comments I have so far.

LGTM. Thanks for fixing this!

Remove iostream.

Rewrite a few magic constants in a different form, hoping this can make it easier to understand what is going on.

Modify test to also check a negative value beyond -max(float).

Fix commit message.

Modify test cases to check for the use of some constants.

Fix the test to pass in a scalar value instead of a pointer.
Remove unnecessary device memory allocation and copy.
Add a call to cudaDeviceSynchronize.

Rename round.cu to test_round.cu.
Fix the intrinsic for double.
Change the test to use assert and a variable to prevent constant folding.

In D59947#1446390, @jlebar wrote:

Does XLA:GPU built with this change pass the exhaustive test you added for kRound after we revert the change that made kRound call into libdevice?

Modify test case to check the return value.

The problem is only reported by ubsan (undefined behavior sanitizer). My test case produces a value of 1.27e+80, which is a value between max(float) and max(double). Without ubsan, llvm opt (compiled with clang for x86) produces the correct result silently. However, since (float)the-double-value has undefined behavior by c++ language, there is no guarantee that the llvm opt produced by any compiler for any platform produces the same result. Do you have suggestion on what is a better test case?

Update commit message.

Add a test case.

@spatel Thanks for your comment! I need to revise my example to better match the real case I am looking at-- in particular, to show that the optimization will increase the number of zext instructions. In this example, there are only two zext instructions. However, if we narrow all the arithmetic operations to i32 we will need four zext instructions. In one of your previous comment, you have suggested to extend TruncInstCombine in aggressive-instcombine. I see the following issues: (1) TruncInstCombine doesn't increase the number of zext/sext instructions. (2) TruncInstCombine currently only handles operations with this properties truncate(op(i64), i32) == op(truncate(i64, i32)), div/rem/right-shift which are needed here aren't part of such operations. We can properly add value range analysis to resolve this though. (3) TruncInstCombine is driven by the truncate instruction in the IR, and there is no such truncate instruction in the case here. To handle the case here (such as the pattern "lshr followed by shl" you mentioned) , is it acceptable to add a new optimization to aggressive-instcombine that can increase the number of zext/sext instructions?

What Sanjoy suggested doesn't (completely) solve the problem we have , where UDIV and UREM are used to calculate the dimensional indices from a linear index, and the dimensionally indices are then used to access tensors of different shapes (for example, two tensors being accessed both require broadcasting but along different dimensions).
With the current state of D47113, would it be acceptable if I revise my change here to only narrow i64 udiv/urem to i32 using the added patterns?

InstCombiner::visitShl performs similar narrowing without checking user count and can increase the total number to ZExt instructions.

InstCombiner::visitLShr can perform the same transformation for the cases where correlated-value propagation is not needed to discover the range of the values.
However, unlike the transformation here, InstCombiner::visitLShr carefully makes sure that the transformation won't increase the total number of ZExt/ZExt instructions (Op1.hasOneUse check). Why the transformation here doesn't need similar check? Is it safe to remove such a check in InstCombiner::visitLShr?

Ping.

Instcombine has udiv/urem narrowing without increasing the total number of ZExt instructions. Correlated-Propagation has udiv/urem narrowing that can increasing the total number of ZExt instruction (kind of a superset of what is in instcombine). These are existing duplicated functionalities. I have considered two possible changes to this: (1) allow instcombine to increase the total number of ZExt instructions in general (by simply removing the m_oneuse related checks). (2) as shown in this change, allow instcombine to handle a special case and may increase the number of ZExt instructions by one.
I looked at other similar narrowing transformations in instcombine, and found most of them have the check to avoid increasing the total number of ZExt/SExt instructions (lshr narrowing for example), but some of them don't have such a check (shl narrowing for example). I am not sure whether these are by design or due to something else such as ignorance. Do you know about this?

Are you suggesting to teach correlated-propagation to narrow shl/and? InstCombine can perform such narrowing, I don't see why it is better to duplicate this in correlated-propagation. Can you explain? The "m_OneUse" check in the code I am changing here is a simple logic to make sure that the transformation do not increase the number of ZExt instructions. I basically add a little more logic there to say "if there are two uses, and we can guarantee that the transformation do not increase the number of ZExt instructions, we also want to perform the transformation here. " What are your concerns for such a change?

Yes, you are right in that correlated value propagation can handle the case. However, if the divisor is a power of 2 and instcombine is invoked before correlated value propagation (as in the opt passes), instcombine transforms the i64 udiv/urem into i64 shift/and. My original motivated test case is like below, the change here is required in order for opt transform all the i64 arithmetic operations into i32 operations.

Use early return.

Add another test case.

• Removed nuw nsw from the tests.

• Removed nuw nsw from the tests.

• Removed nuw nsw from the tests.

• Removed nuw nsw from the tests.

• Removed nuw nsw from the tests.
• Removed \brief from the comment.

• Remove nuw nsw from the tests.

• Applied git clang-format.

• Use prefix ++ instead of postfix ++.

• Use prefix ++ instead of postfix ++.

• Use instnamer to fix names.

• Use instnamer to fix names.

• Use instnamer to fix the name of the tests.

• Add a blank line.