Skip to content

Conversation

@AGindinson
Copy link
Contributor

Avoids crashes for operations like tensor.dim / tensor.from_elements when hoisting constant expressions.

The only HoistableTypeInterface's that are feasible to instantiate are the existing ones, for general tensors and for scalar indices. tensor<index> types need explicit handling, as default utils for bitwidth assessment do not (and should not) assume the exact size of the index type.

The approach taken is to patch up the common RankedTensorType paths in HoistableTypeInterface and ConstExpr analyses. The conservative assumption of 64-bit indices is wrapped around the common helpers like IREE::Util::getRoundedPhysicalStorageSize and IREE::Util::getTypeBitWidth - and therefore, isolated from the rest of the codebase.

Instead of the 64-bit assumption, an alternative approach would be to access module-level data layout information from within ConstExprAnalysis. Achieving this for HoistableTypeInterface seems less clear.

Avoids crashes for operations like `tensor.dim` / `tensor.from_elements` when hoisting constant expressions.

The only `HoistableTypeInterface`'s that are feasible to instantiate are the existing ones, for general tensors and for scalar indices. `tensor<index>` types need explicit handling, as default utils for bitwidth assessment do not (and should not) assume the exact size of the index type.

The approach taken is to patch up the common `RankedTensorType` paths in `HoistableTypeInterface` and `ConstExpr` analyses.
The conservative assumption of 64-bit indices is wrapped around the common helpers like `IREE::Util::getRoundedPhysicalStorageSize` and `IREE::Util::getTypeBitWidth` - and therefore, isolated
from the rest of the codebase.

Instead of the 64-bit assumption, an alternative approach would be to access module-level
data layout information from within `ConstExprAnalysis`. Achieving this for `HoistableTypeInterface`
seems less clear.

Signed-off-by: Artem Gindinson <[email protected]>
Copy link
Collaborator

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

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

It can be tricky handling index bits this way. There could be mismatches if the initializer runs on a machine where index is mapped to 32-bits, and the rest of it maps index to 64-bits (or vice-versa). I dont know off the top of my head what would happen today.

@AGindinson
Copy link
Contributor Author

@MaheshRavishankar Ah, that's a great catch. Intuitively, it seems that my optimization is plain incorrect if the host indexes in 32-bit. I'll have a go at querying TargetInfo then.

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