[PATCH] D76479: [mlir][LLVMIR] Fix fusion for rank-0 tensors

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added inline comments.
This revision is now accepted and ready to land.


================
Comment at: mlir/lib/IR/AffineMap.cpp:260
 
-  return get(numResultDims, numResultSyms, results);
+  return results.empty() ? get(numResultDims, 0, this->getContext())
+                         : get(numResultDims, numResultSyms, results);
----------------
asaadaldien wrote:
> mravishankar wrote:
> > asaadaldien wrote:
> > > mravishankar wrote:
> > > > There are going to be so many of such cases. The get method that gets context from the result expression should just be removed IMO
> > > IMO we should have a single ::get method that handle any results exprs including empty e.g
> > > ```
> > > AffineMap AffineMap::get(unsigned dimCount, unsigned symbolCount,
> > >                          ArrayRef<AffineExpr> results) {
> > >   
> > >   if (!results.empty()) {
> > >   getImpl(dimCount, symbolCount, results, results[0].getContext());
> > >  } else {
> > >    assert(symbolCount == 0);
> > >    return get(numResultDims, 0, this->getContext());
> > >  }
> > > }
> > > ```
> > > Not sure if Nicolas prefer to have an explicit get method for rank-0 tensors for a specific reason.
> > There is no `this->getContext()` . This is a static class method. So this would not compile.
> > 
> > I think the reason for the separate method is because  of the current `assert(!result.empty())`. Should just have a single method
> > 
> > ```
> > AffineMap AffineMap::get(unsigned dimCount, unsigned symbolCount, ArrayRef<AffineExpr> results, MLIRContext *context) {
> >   return getImpl(dimCount, symbolCount, results, context);
> > }
> > ```
> ah Didn't notice its static method. having both results & context and a single get SGTM. We can do it in a follow up diff it will touch many places 
nit: Drop the `this->`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76479