Page MenuHomePhabricator
Differential D75965 Diff 249528 mlir/include/mlir/Transforms/LoopUtils.h

Changeset View

Standalone View

View Options

mlir/include/mlir/Transforms/LoopUtils.h

Show All 9 Lines
// methods: these are not passes by themselves but are used either by passes, // methods: these are not passes by themselves but are used either by passes,
// optimization sequences, or in turn by other transformation utilities. // optimization sequences, or in turn by other transformation utilities.
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#ifndef MLIR_TRANSFORMS_LOOP_UTILS_H #ifndef MLIR_TRANSFORMS_LOOP_UTILS_H
#define MLIR_TRANSFORMS_LOOP_UTILS_H #define MLIR_TRANSFORMS_LOOP_UTILS_H
#include "mlir/IR/Block.h" #include "mlir/IR/Block.h"
rriddleUnsubmitted
Done
ReplyInline Actions

Can you can forward declare the necessary things and move this header to the .cpp file?

rriddle: Can you can forward declare the necessary things and move this header to the .cpp file?
#include "mlir/Support/LLVM.h" #include "mlir/Support/LLVM.h"
#include "mlir/Support/LogicalResult.h" #include "mlir/Support/LogicalResult.h"
namespace mlir { namespace mlir {
class AffineForOp; class AffineForOp;
class FuncOp; class FuncOp;
class OpBuilder; class OpBuilder;
class Value; class Value;
struct MemRefRegion;
namespace loop { namespace loop {
class ForOp; class ForOp;
} // end namespace loop } // end namespace loop
/// Unrolls this for operation completely if the trip count is known to be /// Unrolls this for operation completely if the trip count is known to be
/// constant. Returns failure otherwise. /// constant. Returns failure otherwise.
LogicalResult loopUnrollFull(AffineForOp forOp); LogicalResult loopUnrollFull(AffineForOp forOp);
▲ Show 20 Lines • Show All 145 Lines • ▼ Show 20 Lines
/// A convenience version of affineDataCopyGenerate for all ops in the body of /// A convenience version of affineDataCopyGenerate for all ops in the body of
/// an AffineForOp. /// an AffineForOp.
uint64_t affineDataCopyGenerate(AffineForOp forOp, uint64_t affineDataCopyGenerate(AffineForOp forOp,
const AffineCopyOptions &copyOptions, const AffineCopyOptions &copyOptions,
Optional<Value> filterMemRef, Optional<Value> filterMemRef,
DenseSet<Operation *> &copyNests); DenseSet<Operation *> &copyNests);
/// generateDataCopyAroundOp is similar to affineDataCopyGenerate, but with some
bondhugulaUnsubmitted
Done
ReplyInline Actions

This doc comment should moved down to the method declaration below. Please mention that this performs data copy generation for a single memref region associated with a single op.

bondhugula: This doc comment should moved down to the method declaration below. Please mention that this…
bondhugulaUnsubmitted
Done
ReplyInline Actions

Doc comment here please (even if it's brief).

bondhugula: Doc comment here please (even if it's brief).
/// simplifications:
/// * The logic of "find relavant memrefs and their uses" is de-coupled and push
bondhugulaUnsubmitted
Done
ReplyInline Actions

relevant

bondhugula: relevant
bondhugulaUnsubmitted
Done
ReplyInline Actions

push -> pushed

bondhugula: push -> pushed
/// back to the users. It focuses on generating fast buffers and associated
/// loops/DMAs.
/// * It handles a single memref per call.
bondhugulaUnsubmitted
Done
ReplyInline Actions

buffer -> buffer allocation

bondhugula: buffer -> buffer allocation
/// * The prologue and epilogue always surround `op`, not in potentially
/// arbitrary places.
bondhugulaUnsubmitted
Done
ReplyInline Actions

alloc above is an operation. alloc -> allocated buffer?

bondhugula: `alloc` above is an operation. alloc -> allocated buffer?
///
/// Notice that certain options in `copyOptions` isn't looked at anymore, like
bondhugulaUnsubmitted
Done
ReplyInline Actions

Nit: Notice -> Note that

bondhugula: Nit: Notice -> Note that
/// slowMemorySpace.
struct CopyGenerateResult {
uint64_t sizeInBytes;
bondhugulaUnsubmitted
Done
ReplyInline Actions

generateCopyFromMemRefRegion -> generateCopyForMemRegion ?

bondhugula: generateCopyFromMemRefRegion -> generateCopyForMemRegion ?
bondhugulaUnsubmitted
Done
ReplyInline Actions

This doc comment appears to provides detail before the actual information. Perhaps just rephrase to:

/// Similar to affineDataCopyGenerate, but works on a single memref region.

You could move "The logic of "find ..." as a comment on the definition of the method.

bondhugula: This doc comment appears to provides detail before the actual information. Perhaps just…
timshenAuthorUnsubmitted
Done
ReplyInline Actions

Simplified further. The "The logic of" part looks like design rationales rather than function descriptions, which is repeating after the API itself.

timshen: Simplified further. The "The logic of" part looks like design rationales rather than function…
Operation *alloc;
Operation *copyNest;
bondhugulaUnsubmitted
Done
ReplyInline Actions

Doc comment.

bondhugula: Doc comment.
};
LogicalResult generateDataCopyAroundOp(const MemRefRegion &memrefRegion,
bondhugulaUnsubmitted
Done
ReplyInline Actions

-> a single memref region denoted ...

bondhugula: -> a single memref region denoted ...
Operation *where,
bondhugulaUnsubmitted
Done
ReplyInline Actions

A better name for 'where'?

bondhugula: A better name for 'where'?
const AffineCopyOptions &copyOptions,
CopyGenerateResult &result);
/// Tile a nest of standard for loops rooted at `rootForOp` by finding such /// Tile a nest of standard for loops rooted at `rootForOp` by finding such
/// parametric tile sizes that the outer loops have a fixed number of iterations /// parametric tile sizes that the outer loops have a fixed number of iterations
bondhugulaUnsubmitted
Done
ReplyInline Actions

Nit: isn't -> aren't

bondhugula: Nit: isn't -> aren't
/// as defined in `sizes`. /// as defined in `sizes`.
TileLoops extractFixedOuterLoops(loop::ForOp rootFOrOp, TileLoops extractFixedOuterLoops(loop::ForOp rootFOrOp,
ArrayRef<int64_t> sizes); ArrayRef<int64_t> sizes);
bondhugulaUnsubmitted
Done
ReplyInline Actions

Camel case.

bondhugula: Camel case.
bondhugulaUnsubmitted
Done
ReplyInline Actions

insertionPoint would be used typically for iterator types. Shouldn't this just be a Block::iterator? At the call site, you could pass Block::iterator(op).

bondhugula: insertionPoint would be used typically for iterator types. Shouldn't this just be a Block…
timshenAuthorUnsubmitted
Done
ReplyInline Actions

This op is more than an iterator insertion point. It's also the op that's analyzed by the memref region. It also indicate that something next to the iterator is going to change (std::next(insertionPoint)). Change the name to analyzedOp, and adjusted comments for that.

timshen: This op is more than an iterator insertion point. It's also the op that's analyzed by the…
/// Replace a perfect nest of "for" loops with a single linearized loop. Assumes /// Replace a perfect nest of "for" loops with a single linearized loop. Assumes
/// `loops` contains a list of perfectly nested loops with bounds and steps /// `loops` contains a list of perfectly nested loops with bounds and steps
/// independent of any loop induction variable involved in the nest. /// independent of any loop induction variable involved in the nest.
void coalesceLoops(MutableArrayRef<loop::ForOp> loops); void coalesceLoops(MutableArrayRef<loop::ForOp> loops);
/// Maps `forOp` for execution on a parallel grid of virtual `processorIds` of /// Maps `forOp` for execution on a parallel grid of virtual `processorIds` of
/// size given by `numProcessors`. This is achieved by embedding the SSA values /// size given by `numProcessors`. This is achieved by embedding the SSA values
Show All 39 Lines