[PATCH] D135736: [mlir][MemRef] Make reinterpret_cast(extract_strided_metadata) more robust

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp:1748
+static bool sameValueOrConstantAtIdx(
+    ArrayRef<OpFoldResult> valuesA, ArrayRef<int64_t> constantsA,
+    ArrayRef<OpFoldResult> valuesB, ArrayRef<int64_t> constantsB,
----------------
qcolombet wrote:
> nicolasvasilache wrote:
> > This feels like a very clunky API to use .. 
> > 
> > Can we merge the OFR and int64_t into a single vector keeping the most static information and only directly compare equality ?
> > 
> > I can't tell offhand if whether this is possible or whether the logic allows for more complex mixings of Value vs int comparisons.
> > Can we merge the OFR and int64_t into a single vector keeping the most static information and only directly compare equality ?
> 
> I don't know how much it would help API-wise in the sense that the tricky bit is to get this information to beginning with. (I.e., what is currently done in `sameValueOrConstant` and more particularly the lambda `getConstant`.)
Looking at this again, I don't see a whole lot of simplification without some code duplication (which is what I wanted to avoid in the first place).

The annoying part is, to get the most static information, we need both the `OpFoldResult` (to get the index value or to crack open the related `arith.constant`) and the related `int64_t` that we deduced from the type. I.e., in terms of API, we need both `A` and `constantA` to get the most static information. (And we also  need to know if `constant` is dynamic.)

If we were to introduce a `getMostStaticInformation` it would essentially look like the `getConstant` lambda. Then for the checks themselves, we would have to replicate the logic that compares two "most static information". (I.e. what lives in `sameValueOrConstant` right now.

Ideas?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135736/new/

https://reviews.llvm.org/D135736