Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle out-of-bound value for Gather alike operation #3077

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chentong319
Copy link
Collaborator

Current situation: when a value of indices is out-of-bound, the execution is halted with assertion error.
This PR provides some improvements:

  1. Do not generate assertion error. Instead, print out error message and replace with out-of-bound value with zero, with the hope that the execution will finish gracefully.
  2. Print the out-of-bound value.
  3. Share the code for GatherOp and GatherElementsOp. I did not define a common utility function because there are too many parameters. In future, we can merge all the lowering code, not just for enable-safe-code-gen, for the two ops.

Signed-off-by: Chen Tong <[email protected]>
Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

You could do a common function

generateDynBoundCheck0ToUb(rewriter, op, index, ub, message)

where you extract the message you want from the op, can append message, and generate the needed code.

" indices of GatherOp is less than the lower bound");
create.math.slt(index.getValue(), zeroIE.getValue());
Value outBound =
create.math.ori(compareUpperBound, compareLowerBound);
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add SCFBuilder to create object, and then you can use

  void ifThenElse(mlir::Value cond, SCFThenElseBodyFn thenFn,
      SCFThenElseBodyFn elseFn = nullptr) const;

to be in line with current practices.

// The modification of index could be put into IfOp to save
// some condition check, but the code will become complicated.
index = index.selectOrSelf(index < zeroIE, zeroIE);
index = index.selectOrSelf(index >= axisDim, zeroIE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not capping at 0 on the lower side and axisDim on the upper size?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No particular reason. Just feel 0 is a safer choice.

rewriter.create<cf::AssertOp>(loc, compareUpperBound,
nodeNameStr +
" indices of GatherOp is larger than the upper bound");

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since index is already an IndexExpr, and that we have the dim of axis in the loop bounds, you could use index express all the way through as it has overloaded operators.

  IndexExpr operator==(IndexExpr const b) const;
  IndexExpr operator==(int64_t const b) const;
  IndexExpr operator!=(IndexExpr const b) const;
  IndexExpr operator!=(int64_t const b) const;
  IndexExpr operator<=(IndexExpr const b) const;
  IndexExpr operator<=(int64_t const b) const;
  IndexExpr operator<(IndexExpr const b) const;
  IndexExpr operator<(int64_t const b) const;
  IndexExpr operator>=(IndexExpr const b) const;
  IndexExpr operator>=(int64_t const b) const;
  IndexExpr operator>(IndexExpr const b) const;
  IndexExpr operator>(int64_t const b) const;

  IndexExpr operator&(IndexExpr const b) const;
  IndexExpr operator|(IndexExpr const b) const;
  IndexExpr operator!() const;

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

Successfully merging this pull request may close these issues.

2 participants