Skip to content

Conversation

@SwapnilGhanshyala
Copy link

Updated AffineParallelLowering to check if reduction op value being returned is valid else return failure in matchAndRewrite Updated ArithOps.cpp getIdentityValue method to return nullptr if op is not a valid reduction op Code cleanup in ArithOps.cpp getReductionOp method; removed cases maxnumf and minnumf as not valid reduction ops

Reporting Issue is llvm#64073

@SwapnilGhanshyala
Copy link
Author

This crash occurred when getIdentityValue tries to build a value based on the 'attr' recieved, which is a nullptr if the op is not a valid reduction op. In llvm#64073 the reduction op being passed is 'assign' which although is atomicRMW but not reduction op.
The assert in AffineParallelLowering which was meant to catch this invalid reduction op before it was passed to getIdentityValue, fails. This is because the AtomicRMWKind enum/list of ops that was checked for the validity of reduction op. This enum returns 'assign' as a valid reduction op, therefore, the assert passes in the receiver.

In the current fix, the getIdentityValue returns a nullptr instead of building a value if the op is invalid.

Another proposed fix could be to have a separate enum like 'AtomicRMWReductionKind' to hold only reduction ops of atomicRMW kind. It would be easier to work with reduction ops using this enum.
Currently the approach is identify AtomicRMWKind ops and use 'getIdentityValue' as a rough filter.

Comment on lines -2528 to 2530

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this deleted?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both maxnumf and minnumf are invalid reduction ops and should not be a valid case in getReductionOps.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

@SwapnilGhanshyala SwapnilGhanshyala force-pushed the issue_64073 branch 2 times, most recently from 5224420 to a353dc8 Compare October 26, 2023 08:16
Updated AffineParallelLowering to check if reduction op value being returned is valid else return failure in matchAndRewrite
Updated ArithOps.cpp getIdentityValue method to return nullptr if op is not a valid reduction op
Code cleanup in ArithOps.cpp getReductionOp method; removed cases maxnumf and minnumf as not valid reduction ops

Reporting Issue is llvm#64073
@SwapnilGhanshyala
Copy link
Author

Changes addressed :

  1. variable name readability
  2. unrelated clang-formatting

OpBuilder &builder, Location loc,
bool useOnlyFiniteValue) {
auto attr =
getIdentityValueAttr(op, resultType, builder, loc, useOnlyFiniteValue);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressing comment on lines 2528 to 2530 regarding deletion of maxnumf and minnumf as reduction op cases:

The 'getIdentityAttr()' method called here returns valid attr iff the op passed is a valid reductionOp else returns nullptr.
This method does not consider 'maxnumf' and 'minnumf' as valid reduction ops. If this is correct, then consequently maxnumf and minnumf cases should be safely removable from the "getReductionOp()", as done in this commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants