[PATCH] D136483: [mlir][MemRefToLLVM] Reuse existing lowering for collaspe/expand_shape

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

> So if I understand correctly, we are now emitting more arithmetics that we are unable to simplify at the MLIR level.

That's correct.

> This sounds concerning and seem to defy the purpose of using affine maps, which should be the easy-to-compose closed-form expressions.

When I first wrote this lowering (see https://reviews.llvm.org/D133166 for the details.), we started with doing compositions, but I found that it made the resulting IR hard to reason about. Now that I am more familiar with MLIR and affine maps in particular, maybe this is not as bad. I.e., we could revert this decision if that cause any problem.

> Do we expect LLVM's CSE to simplify this?

Yes, I would expect LLVM's CSE to pick it up since we are dealing with simple math operations.

> Is there any indication of this actually happening or not?

I haven't actually checked. I'll do that.
How do you go from the llvm dialect to actual LLVM IR?

That said, this is an interesting issue. Do we expect MLIR to produce the most optimized/concise code as possible or do/can we rely on the lower layers to do the cleanups?

If this is the former, then for instance why are we lowering dead code to beginning with?

> (In one my previous projects, we've seen a performance improvement attributable to better/simpler address generation via memrefs, which we may now undo...)

Let's double check if the CSE happens right now or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136483