This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVMIR] Fix fusion for rank-0 tensors
ClosedPublic

Authored by asaadaldien on Mar 20 2020, 12:01 AM.

Details

Summary

This diff fixes fusion craching for ops with rank-0 tensors

Diff Detail

Event Timeline

asaadaldien created this revision.Mar 20 2020, 12:01 AM
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache accepted this revision.Mar 20 2020, 6:13 AM
mravishankar accepted this revision.Mar 20 2020, 9:20 AM
mravishankar added inline comments.
mlir/lib/IR/AffineMap.cpp
260

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

asaadaldien marked an inline comment as done.Mar 20 2020, 10:07 AM
asaadaldien added inline comments.
mlir/lib/IR/AffineMap.cpp
260

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.

mravishankar added inline comments.Mar 20 2020, 10:20 AM
mlir/lib/IR/AffineMap.cpp
260

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);
}
asaadaldien marked an inline comment as done.Mar 20 2020, 10:58 AM
asaadaldien added inline comments.
mlir/lib/IR/AffineMap.cpp
260

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

rriddle accepted this revision.Mar 20 2020, 12:34 PM
rriddle added inline comments.
mlir/lib/IR/AffineMap.cpp
260

nit: Drop the this->

This revision is now accepted and ready to land.Mar 20 2020, 12:34 PM
asaadaldien marked an inline comment as done.Mar 20 2020, 12:44 PM
This revision was automatically updated to reflect the committed changes.