[RFC][mlir] ViewLikeOpInterface method for detecting partial views. #164020
Conversation
vzakhari
requested review from
Renaud-K,
River707,
jeanPerier,
joker-eph and
razvanlupusoru
llvmbot
added
mlir
flang
|
@llvm/pr-subscribers-mlir-memref @llvm/pr-subscribers-mlir Author: Slava Zakharin (vzakhari) ChangesI am trying to simplify the implementation of Flang's AliasAnalysis In this patch, I only add ViewLikeOpInterface support to few As soon as I add, ViewLikeOpInterface interface support for FIR This is what the new method I would like to fix both LocalAliasAnalysis and Flang's alias analysis If my understanding of Full diff: https://github.com/llvm/llvm-project/pull/164020.diff 5 Files Affected:
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.h b/flang/include/flang/Optimizer/Dialect/FIROps.h
index 62ef8b4b502f2..4651f2bb8038e 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.h
@@ -20,6 +20,7 @@
#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/Interfaces/LoopLikeInterface.h"
#include "mlir/Interfaces/SideEffectInterfaces.h"
+#include "mlir/Interfaces/ViewLikeInterface.h"
namespace fir {
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index fc6eedc6ed4c6..5ce7390fca0d8 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -17,6 +17,7 @@
include "mlir/Dialect/Arith/IR/ArithBase.td"
include "mlir/Dialect/Arith/IR/ArithOpsInterfaces.td"
include "mlir/Dialect/LLVMIR/LLVMAttrDefs.td"
+include "mlir/Interfaces/ViewLikeInterface.td"
include "flang/Optimizer/Dialect/CUF/Attributes/CUFAttr.td"
include "flang/Optimizer/Dialect/FIRDialect.td"
include "flang/Optimizer/Dialect/FIRTypes.td"
@@ -1730,8 +1731,9 @@ def fir_ArrayMergeStoreOp : fir_Op<"array_merge_store",
// Record and array type operations
//===----------------------------------------------------------------------===//
-def fir_ArrayCoorOp : fir_Op<"array_coor",
- [NoMemoryEffect, AttrSizedOperandSegments]> {
+def fir_ArrayCoorOp
+ : fir_Op<"array_coor", [NoMemoryEffect, AttrSizedOperandSegments,
+ ViewLikeOpInterface]> {
let summary = "Find the coordinate of an element of an array";
@@ -1774,9 +1776,13 @@ def fir_ArrayCoorOp : fir_Op<"array_coor",
let hasVerifier = 1;
let hasCanonicalizer = 1;
+ let extraClassDeclaration = [{
+ mlir::Value getViewSource() { return getMemref(); }
+ }];
}
-def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
+def fir_CoordinateOp
+ : fir_Op<"coordinate_of", [NoMemoryEffect, ViewLikeOpInterface]> {
let summary = "Finds the coordinate (location) of a value in memory";
@@ -1828,6 +1834,7 @@ def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
let extraClassDeclaration = [{
constexpr static int32_t kDynamicIndex = std::numeric_limits<int32_t>::min();
CoordinateIndicesAdaptor getIndices();
+ mlir::Value getViewSource() { return getRef(); }
}];
}
@@ -2792,7 +2799,8 @@ def fir_VolatileCastOp : fir_SimpleOneResultOp<"volatile_cast", [Pure]> {
let hasFolder = 1;
}
-def fir_ConvertOp : fir_SimpleOneResultOp<"convert", [NoMemoryEffect]> {
+def fir_ConvertOp
+ : fir_SimpleOneResultOp<"convert", [NoMemoryEffect, ViewLikeOpInterface]> {
let summary = "encapsulates all Fortran entity type conversions";
let description = [{
@@ -2830,6 +2838,8 @@ def fir_ConvertOp : fir_SimpleOneResultOp<"convert", [NoMemoryEffect]> {
static bool isPointerCompatible(mlir::Type ty);
static bool canBeConverted(mlir::Type inType, mlir::Type outType);
static bool areVectorsCompatible(mlir::Type inTy, mlir::Type outTy);
+ mlir::Value getViewSource() { return getValue(); }
+ bool isSameStart() { return true; }
}];
let hasCanonicalizer = 1;
}
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index 73ddd1ff80126..f3470ecaa0ffc 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -559,10 +559,19 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
type = SourceKind::Allocate;
breakFromLoop = true;
})
- .Case<fir::ConvertOp>([&](auto op) {
- // Skip ConvertOp's and track further through the operand.
- v = op->getOperand(0);
+ .Case<mlir::ViewLikeOpInterface>([&](auto op) {
+ if (isPointerReference(ty))
+ attributes.set(Attribute::Pointer);
+ v = op.getViewSource();
defOp = v.getDefiningOp();
+ // If the source is a box, and the result is not a box,
+ // then this is one of the box "unpacking" operations,
+ // so we should set followingData.
+ if (mlir::isa<fir::BaseBoxType>(v.getType()) &&
+ !mlir::isa<fir::BaseBoxType>(ty))
+ followingData = true;
+ if (!op.isSameStart())
+ approximateSource = true;
})
.Case<fir::PackArrayOp>([&](auto op) {
// The packed array is not distinguishable from the original
@@ -578,15 +587,6 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
if (mlir::isa<fir::BaseBoxType>(v.getType()))
followBoxData = true;
})
- .Case<fir::ArrayCoorOp, fir::CoordinateOp>([&](auto op) {
- if (isPointerReference(ty))
- attributes.set(Attribute::Pointer);
- v = op->getOperand(0);
- defOp = v.getDefiningOp();
- if (mlir::isa<fir::BaseBoxType>(v.getType()))
- followBoxData = true;
- approximateSource = true;
- })
.Case<fir::EmboxOp, fir::ReboxOp>([&](auto op) {
if (followBoxData) {
v = op->getOperand(0);
diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.td b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
index 131c1a0d92b24..5903623572b6e 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.td
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
@@ -23,21 +23,34 @@ def ViewLikeOpInterface : OpInterface<"ViewLikeOpInterface"> {
}];
let cppNamespace = "::mlir";
- let methods = [
- InterfaceMethod<
- "Returns the source buffer from which the view is created.",
- "::mlir::Value", "getViewSource">,
- InterfaceMethod<
- /*desc=*/[{ Returns the buffer which the view created. }],
- /*retTy=*/"::mlir::Value",
- /*methodName=*/"getViewDest",
- /*args=*/(ins),
- /*methodBody=*/"",
- /*defaultImplementation=*/[{
+ let methods =
+ [InterfaceMethod<
+ "Returns the source buffer from which the view is created.",
+ "::mlir::Value", "getViewSource">,
+ InterfaceMethod<
+ /*desc=*/[{ Returns the buffer which the view created. }],
+ /*retTy=*/"::mlir::Value",
+ /*methodName=*/"getViewDest",
+ /*args=*/(ins),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
return $_op->getResult(0);
- }]
- >
- ];
+ }]>,
+ InterfaceMethod<
+ // TBD: should it be generalized into isPartialView that will
+ // return true in any of these cases?
+ // 1. Resulting view is a slice view of the source.
+ // 2. Resulting view's start does not match the source's start.
+ // 3. Resulting view's end does not match the source's end.
+ /*desc=*/
+ [{ Returns true iff the source buffer and the resulting view start at the same "address". }],
+ /*retTy=*/"bool",
+ /*methodName=*/"isSameStart",
+ /*args=*/(ins),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
+ return false;
+ }]>];
}
def OffsetSizeAndStrideOpInterface : OpInterface<"OffsetSizeAndStrideOpInterface"> {
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index a84d10d5d609d..b0ec97bd9f264 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -129,6 +129,10 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
// If this is a view, unwrap to the source.
if (ViewLikeOpInterface view = dyn_cast<ViewLikeOpInterface>(op)) {
if (result == view.getViewDest()) {
+ // TODO: if view.isSameStart() returns false here,
+ // we have to make sure that further analysis may return
+ // PartialAlias for all underlying addresses we are going
+ // to collect from this point up the def-use.
return collectUnderlyingAddressValues(view.getViewSource(), maxDepth,
visited, output);
}
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Slava Zakharin (vzakhari) ChangesI am trying to simplify the implementation of Flang's AliasAnalysis In this patch, I only add ViewLikeOpInterface support to few As soon as I add, ViewLikeOpInterface interface support for FIR This is what the new method I would like to fix both LocalAliasAnalysis and Flang's alias analysis If my understanding of Full diff: https://github.com/llvm/llvm-project/pull/164020.diff 5 Files Affected:
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.h b/flang/include/flang/Optimizer/Dialect/FIROps.h
index 62ef8b4b502f2..4651f2bb8038e 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.h
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.h
@@ -20,6 +20,7 @@
#include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
#include "mlir/Interfaces/LoopLikeInterface.h"
#include "mlir/Interfaces/SideEffectInterfaces.h"
+#include "mlir/Interfaces/ViewLikeInterface.h"
namespace fir {
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index fc6eedc6ed4c6..5ce7390fca0d8 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -17,6 +17,7 @@
include "mlir/Dialect/Arith/IR/ArithBase.td"
include "mlir/Dialect/Arith/IR/ArithOpsInterfaces.td"
include "mlir/Dialect/LLVMIR/LLVMAttrDefs.td"
+include "mlir/Interfaces/ViewLikeInterface.td"
include "flang/Optimizer/Dialect/CUF/Attributes/CUFAttr.td"
include "flang/Optimizer/Dialect/FIRDialect.td"
include "flang/Optimizer/Dialect/FIRTypes.td"
@@ -1730,8 +1731,9 @@ def fir_ArrayMergeStoreOp : fir_Op<"array_merge_store",
// Record and array type operations
//===----------------------------------------------------------------------===//
-def fir_ArrayCoorOp : fir_Op<"array_coor",
- [NoMemoryEffect, AttrSizedOperandSegments]> {
+def fir_ArrayCoorOp
+ : fir_Op<"array_coor", [NoMemoryEffect, AttrSizedOperandSegments,
+ ViewLikeOpInterface]> {
let summary = "Find the coordinate of an element of an array";
@@ -1774,9 +1776,13 @@ def fir_ArrayCoorOp : fir_Op<"array_coor",
let hasVerifier = 1;
let hasCanonicalizer = 1;
+ let extraClassDeclaration = [{
+ mlir::Value getViewSource() { return getMemref(); }
+ }];
}
-def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
+def fir_CoordinateOp
+ : fir_Op<"coordinate_of", [NoMemoryEffect, ViewLikeOpInterface]> {
let summary = "Finds the coordinate (location) of a value in memory";
@@ -1828,6 +1834,7 @@ def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
let extraClassDeclaration = [{
constexpr static int32_t kDynamicIndex = std::numeric_limits<int32_t>::min();
CoordinateIndicesAdaptor getIndices();
+ mlir::Value getViewSource() { return getRef(); }
}];
}
@@ -2792,7 +2799,8 @@ def fir_VolatileCastOp : fir_SimpleOneResultOp<"volatile_cast", [Pure]> {
let hasFolder = 1;
}
-def fir_ConvertOp : fir_SimpleOneResultOp<"convert", [NoMemoryEffect]> {
+def fir_ConvertOp
+ : fir_SimpleOneResultOp<"convert", [NoMemoryEffect, ViewLikeOpInterface]> {
let summary = "encapsulates all Fortran entity type conversions";
let description = [{
@@ -2830,6 +2838,8 @@ def fir_ConvertOp : fir_SimpleOneResultOp<"convert", [NoMemoryEffect]> {
static bool isPointerCompatible(mlir::Type ty);
static bool canBeConverted(mlir::Type inType, mlir::Type outType);
static bool areVectorsCompatible(mlir::Type inTy, mlir::Type outTy);
+ mlir::Value getViewSource() { return getValue(); }
+ bool isSameStart() { return true; }
}];
let hasCanonicalizer = 1;
}
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index 73ddd1ff80126..f3470ecaa0ffc 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -559,10 +559,19 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
type = SourceKind::Allocate;
breakFromLoop = true;
})
- .Case<fir::ConvertOp>([&](auto op) {
- // Skip ConvertOp's and track further through the operand.
- v = op->getOperand(0);
+ .Case<mlir::ViewLikeOpInterface>([&](auto op) {
+ if (isPointerReference(ty))
+ attributes.set(Attribute::Pointer);
+ v = op.getViewSource();
defOp = v.getDefiningOp();
+ // If the source is a box, and the result is not a box,
+ // then this is one of the box "unpacking" operations,
+ // so we should set followingData.
+ if (mlir::isa<fir::BaseBoxType>(v.getType()) &&
+ !mlir::isa<fir::BaseBoxType>(ty))
+ followingData = true;
+ if (!op.isSameStart())
+ approximateSource = true;
})
.Case<fir::PackArrayOp>([&](auto op) {
// The packed array is not distinguishable from the original
@@ -578,15 +587,6 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
if (mlir::isa<fir::BaseBoxType>(v.getType()))
followBoxData = true;
})
- .Case<fir::ArrayCoorOp, fir::CoordinateOp>([&](auto op) {
- if (isPointerReference(ty))
- attributes.set(Attribute::Pointer);
- v = op->getOperand(0);
- defOp = v.getDefiningOp();
- if (mlir::isa<fir::BaseBoxType>(v.getType()))
- followBoxData = true;
- approximateSource = true;
- })
.Case<fir::EmboxOp, fir::ReboxOp>([&](auto op) {
if (followBoxData) {
v = op->getOperand(0);
diff --git a/mlir/include/mlir/Interfaces/ViewLikeInterface.td b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
index 131c1a0d92b24..5903623572b6e 100644
--- a/mlir/include/mlir/Interfaces/ViewLikeInterface.td
+++ b/mlir/include/mlir/Interfaces/ViewLikeInterface.td
@@ -23,21 +23,34 @@ def ViewLikeOpInterface : OpInterface<"ViewLikeOpInterface"> {
}];
let cppNamespace = "::mlir";
- let methods = [
- InterfaceMethod<
- "Returns the source buffer from which the view is created.",
- "::mlir::Value", "getViewSource">,
- InterfaceMethod<
- /*desc=*/[{ Returns the buffer which the view created. }],
- /*retTy=*/"::mlir::Value",
- /*methodName=*/"getViewDest",
- /*args=*/(ins),
- /*methodBody=*/"",
- /*defaultImplementation=*/[{
+ let methods =
+ [InterfaceMethod<
+ "Returns the source buffer from which the view is created.",
+ "::mlir::Value", "getViewSource">,
+ InterfaceMethod<
+ /*desc=*/[{ Returns the buffer which the view created. }],
+ /*retTy=*/"::mlir::Value",
+ /*methodName=*/"getViewDest",
+ /*args=*/(ins),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
return $_op->getResult(0);
- }]
- >
- ];
+ }]>,
+ InterfaceMethod<
+ // TBD: should it be generalized into isPartialView that will
+ // return true in any of these cases?
+ // 1. Resulting view is a slice view of the source.
+ // 2. Resulting view's start does not match the source's start.
+ // 3. Resulting view's end does not match the source's end.
+ /*desc=*/
+ [{ Returns true iff the source buffer and the resulting view start at the same "address". }],
+ /*retTy=*/"bool",
+ /*methodName=*/"isSameStart",
+ /*args=*/(ins),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
+ return false;
+ }]>];
}
def OffsetSizeAndStrideOpInterface : OpInterface<"OffsetSizeAndStrideOpInterface"> {
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index a84d10d5d609d..b0ec97bd9f264 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -129,6 +129,10 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
// If this is a view, unwrap to the source.
if (ViewLikeOpInterface view = dyn_cast<ViewLikeOpInterface>(op)) {
if (result == view.getViewDest()) {
+ // TODO: if view.isSameStart() returns false here,
+ // we have to make sure that further analysis may return
+ // PartialAlias for all underlying addresses we are going
+ // to collect from this point up the def-use.
return collectUnderlyingAddressValues(view.getViewSource(), maxDepth,
visited, output);
}
|
There was a problem hiding this comment.
It makes sense to me to add the ViewLikeOpInterface to these FIR operations, thanks Slava.
If my understanding of PartialAlias is correct, then we can have a partial alias even when the starting addresses of the source and the resulting view are the same - though, I might be wrong.
That is also my reading of LLVM/MLIR documentation, although I have little experience with how it is used in practice.
|
Hi Slava! Thank you for your proposal! I agree with your points:
Core issue: ViewLikeOpInterface is overloaded Looking at the MemRef dialect, Category 1: Complete alias (same entity, different type):
Category 2: Partial overlap (subset of entity):
Thus, when Proposed solution: I suggest introducing a
This would make Operations like What do you think? |
|
Thank you for the comments, Jean and Razvan. Regarding I thought a bit more about this, and I think that for aliasing analysis we should only care about the beginning of the input and result buffers. For example, if the initial view is Maybe, it is worth having the following two methods in the
|
Interesting - I double-checked against LLVM and its documentation for However, I would like to point out that even LLVM's implementations for alias analysis are not strict about this. For example, a quick search revealed at least one spot where the same base address returns To me, it makes more sense to return Regarding your suggestion to capture all of this in Thank you for your proposal, Slava! |
Thanks for the example! I am not sure if the code actually implies "the same base address returns PartialAlias". I believe in this code, |
|
Here is what I have in the latest commit: I tried not to change the current LocalAliasAnalysis implementation too much. There are still cases where it incorrectly reports MustAlias instead of PartialAlias. Moreover, the order of visiting affects whether MustAlias or PartialAlias is returned. @River707 do you have any comments? |
| // TODO: we should return PartialAlias here. Need to decide | ||
| // if returning PartialAlias for fir.pack_array is okay; | ||
| // if not, then we need to handle it specially to still return | ||
| // MayAlias. |
There was a problem hiding this comment.
From the LLVM documentation:
The PartialAlias response is used when the two memory objects are known to be overlapping in some way, regardless of whether they start at the same address or not.
I guess it depends what exactly is meant by "object". Previously I had interpreted it as referring to the specific memory addresses which could be accessed. So array(1:4) and array(3) do overlap with each other but not with array(100:200). Therefore I think array indexing (until analysed more carefully) should use MayAlias rather than PartialAlias.
Alternatively, if "object" means the entire allocation then PartialAlias makes sense here.
There was a problem hiding this comment.
Thanks for looking, Tom!
I believe in LLVM alias analysis an "object" is defined at a high level as a starting address and the size that are passed to the alias API. So, indeed, the accesses array(1:4) and array(3) do partially overlap, and they do not alias with array(100:200). Now, from the implementation point of view, an alias query may be made with an "infinite" size (up to the end of the underlying memory object). In this case, array(1:4), array(3) and array(100:200) all partially overlap.
When the starting addresses of the two accesses are not compilation constants and cannot be proven to be the same/different, then they may be in both MustAlias and PartialAlias states, so returning MayAlias seems reasonable.
The current implementation of MLIR alias analysis does not accept the access size and I assume that it is "infinite", so returning PartialAlias for such accesses seems conservatively correct to me.
At the same time, maybe I should drop my changes to return PartialAlias in LocalAliasAnalysis and always return MayAlias to allow further alias analyses to make up a more precise answer.
There was a problem hiding this comment.
Thinking about it some more, I think local alias analysis should at least return MayAlias. As I understand it, the distinction between MayAlias and PartialAlias is that a MayAlias result invites further analysis by other methods whereas PartialAlias is considered a definate "these accesses are proven to overlap" and so does not need further analysis if other alias analysis implementations are available.
There was a problem hiding this comment.
I agree with you, Tom. I think the current algorithm in LocalAliasAnalysis (which I am not trying to affect too much, because this is not my goal) does not allow reasoning about partial aliases quite precisely. So I feel okay returning MayAlias in cases where there might be an offset involved. At least, this makes the analysis conservatively correct.
|
The latest commit fixes the visitation so that the order does not matter, and returns |
| /*args=*/(ins), | ||
| /*methodBody=*/"", | ||
| /*defaultImplementation=*/[{ | ||
| return false; |
There was a problem hiding this comment.
Is it safe for this to have a default implementation? Same question for isCompleteView.
There was a problem hiding this comment.
I added the default implementations so that they are conservatively correct, assuming that the methods will be used to perform some transformations based on the fact that a view starts at the same address as the source or/and a view is a complete view of the source.
Maybe, the names of the methods are confusing. Would it be more clear if I named them knownToHaveSameStart and knownToBeCompleteView? The idea is that when the methods return false, one cannot assume whether the view have or not have the same start, etc.
| if (!getOffsets().empty()) | ||
| return false; | ||
| ArrayRef<int64_t> staticOffsets = getStaticOffsets(); | ||
| return llvm::all_of(staticOffsets, |
There was a problem hiding this comment.
What if the offsets are dynamically zero? How can you guarantee this?
/// Return true iff the input and the resulting views start
/// at the same address.
There was a problem hiding this comment.
As in the reply above, the comment should probably say:
/// Return true iff the input and the resulting views **are known to** start
/// at the same address.
Does that look better?
|
Thanks for looking at this PR, @matthias-springer! I appreciate the feedback. |
I am trying to simplify the implementation of Flang's AliasAnalysis by using ViewLikeOpInterface for some FIR dialect operations. Currently, Flang's AliasAnalysis implementation explicitly recognizes specific set of operations, which is inconvenient. ViewLikeOpInterface may also be used in other parts of Flang. This patch adds isKnownToHaveSameStart() method to detect cases where the input and resulting views may be different by an offset, in which case FIR alias analysis must not return `MustAlias`. New method isKnownToBeCompleteView() returns true iff the resulting view is a complete view of the input view. This method will be used by the following patches for OpenACC support in Flang. This patch also modifies LocalAliasAnalysis to stop returning `MustAlias` in cases where both `MustAlias` and `PartialAlias` are possible - we will return `MayAlias` now, which is conservatively correct.
vzakhari
force-pushed
the
viewlikeopinterface_ext1
branch
from
65f9d6c to
afed03e
Compare
There was a problem hiding this comment.
With the default conservative behavior of returning false of isKnownToBeCompleteView, I worry it is "incomplete" (for my own needs not for Alias Analysis). The missing information that I was hoping to rely on is the property of isKnownToBePartialView. I had assumed previously with the addition of isCompleteView that returning false would mean I could make that assumption.
FWIW though I am comfortable with the current changes and thus approving to let the maintainers know you have additional support because I believe this is heading in the right direction to hold additional information critical for AA. Thank you!
For OpenACC clause ordering, such as maintaining appropriate parent-child relationship ordering, we need to be able to walk references back to their base entities. This introduces the operation interface in the `acc` dialect named `PartialEntityAccessOpInterface` which can be used for this purpose. The interface provides two methods: - `getBaseEntity()`: Returns the base entity being accessed - `isCompleteView()`: Indicates whether the access covers the complete entity to allow this interface to be attached to cases that only conditionally offer a partial view This also adds a utility function `mlir::acc::getBaseEntity()` that uses this interface to retrieve the base entity from a value. This work has some similarities with the ViewLikeOpInterface proposal for FIR: llvm#164020 but it differs in the following ways: - Attached only to operations where we can assume a partial entity access - Includes fir.declare operations due to common block storage associations Tests are included that demonstrate the interface on memref.subview operations, implemented locally in the test since memref operations already have ViewLikeOpInterface for similar purposes.
…165911) For OpenACC clause ordering, such as maintaining appropriate parent-child relationship ordering, we need to be able to walk references back to their base entities. This introduces the operation interface in the `acc` dialect named `PartialEntityAccessOpInterface` which can be used for this purpose. The interface provides two methods: - `getBaseEntity()`: Returns the base entity being accessed - `isCompleteView()`: Indicates whether the access covers the complete entity to allow this interface to be attached to cases that only conditionally offer a partial view This also adds a utility function `mlir::acc::getBaseEntity()` that uses this interface to retrieve the base entity from a value. This work has some similarities with the ViewLikeOpInterface proposal for FIR: #164020 but it differs in the following ways: - Attached only to operations where we can assume a partial entity access - Includes fir.declare operations due to common block storage associations Tests are included that demonstrate the interface on memref.subview operations, implemented locally in the test since memref operations already have ViewLikeOpInterface for similar purposes.
…y accesses (#165911) For OpenACC clause ordering, such as maintaining appropriate parent-child relationship ordering, we need to be able to walk references back to their base entities. This introduces the operation interface in the `acc` dialect named `PartialEntityAccessOpInterface` which can be used for this purpose. The interface provides two methods: - `getBaseEntity()`: Returns the base entity being accessed - `isCompleteView()`: Indicates whether the access covers the complete entity to allow this interface to be attached to cases that only conditionally offer a partial view This also adds a utility function `mlir::acc::getBaseEntity()` that uses this interface to retrieve the base entity from a value. This work has some similarities with the ViewLikeOpInterface proposal for FIR: llvm/llvm-project#164020 but it differs in the following ways: - Attached only to operations where we can assume a partial entity access - Includes fir.declare operations due to common block storage associations Tests are included that demonstrate the interface on memref.subview operations, implemented locally in the test since memref operations already have ViewLikeOpInterface for similar purposes.
| are known to start at the same "address". | ||
| }], | ||
| /*retTy=*/"bool", | ||
| /*methodName=*/"isKnownToHaveSameStart", |
There was a problem hiding this comment.
Can isKnownToHaveSameStart and isKnownToBeCompleteView be replaced by a single interface method that returns AliasResult::Kind?
There was a problem hiding this comment.
Yes, I think adding a method like alias will allow me to make the same changes in the alias analyses.
At the same time, I think the new alias method may become redundant if we decide to add the two more "basic" methods like isKnownToHaveSameStart/isKnownToBeCompleteView in future.
I think the following pieces of "basic" information provide everything we need to make a decision about the aliasing of the input and resulting views:
- Can we prove that the resulting view is a complete view of the input view? This method may return
trueeven in cases when the views have dynamic sizes, e.g. based on the semantics of the concrete operation. - Constant size of the input and the resulting views (or nullopt).
- Constant offset applied to the resulting input view to produce the resulting view (or nullopt).
isKnownToHaveSameStartis a simplified version of this query, i.e. it only says if the offset is zero and does not care to compute the actual non-zero constant offset, if it is possible.
I believe having these "basic" methods available, we can implement alias for all operations with ViewLikeOpInterface, and the concrete operations won't have any extra knowledge that will make sense overriding the alias method. That is why I decided to go with more "basic" methods (that may also be useful outside of the alias analysis, probably).
With that said, I agree replacing the two methods with an alias method. We can always adjust it in future if we end up adding all the method providing the "basic" information.
I can think of the following specification for the new alias method:
- This method returns AliasResult::Kind representing the aliasing between the input and the resulting views of a
ViewLikeOpInterfaceoperation. - It returns
NoAliasonly when the resulting view is known to have zero size. Note that when the input view has zero size the resulting view must have zero size as well. Otherwise, - it returns
MustAliaswhen the resulting view is known to start at the same address as the input view. Otherwise, - it returns
PartialViewwhen the resulting view is known to start at a non-zero offset of the input view. Otherwise, - it returns
MayAlias.
Does this definition make sense to you?
There was a problem hiding this comment.
I just noticed that isKnownToBeCompleteView is not expressible with the current AliasingResult::Kind. What I would recommend: Add a new FullAlias to AliasResult::Kind. Add documentation that this is not available in LLVM and that it's a superset of MustAlias. Existing code that looks for MustAlias should now use for MustAlias or FullAlias.
I believe having these "basic" methods available, we can implement alias for all operations with ViewLikeOpInterface, and the concrete operations won't have any extra knowledge that will make sense overriding the alias method.
I don't follow yet. What's the benefit of having "basic" methods if everything is expressible by a single method that returns AliasResult::Kind? The reason why I'm advocating for a single method is because we have an existing API that we can reuse with minor modifications. We don't need to invent a new one.
It returns NoAlias only when the resulting view is known to have zero size.
Is that actually correct according to the LLVM definition of these enum values? Even if both memrefs have size 0, their pointers could be the same. This sounds like MustAlias to me. (Or if we cannot prove that the two pointers are the same, it sounds like MayAlias to me.)
At the same time, LLVM's definition talks about: Another is when the two pointers are only ever used for reading memory. I'm not sure what to make of this. If the two memrefs have size 0, you can neither read nor write from it. Can we start with a more conservative implementation of MayAlias in this case, until we figure out the details?
Note that when the input view has zero size the resulting view must have zero size as well.
This is not necessarily the case. memref.reinterpret_cast implements this interface and it can return a larger memref. (Maybe it shouldn't implement the interface...)
| .Case<fir::ConvertOp>([&](auto op) { | ||
| // Skip ConvertOp's and track further through the operand. | ||
| v = op->getOperand(0); | ||
| .Case<mlir::ViewLikeOpInterface>([&](auto op) { |
There was a problem hiding this comment.
Why does Flang have its own alias analysis? How is it different from the MLIR alias analysis? Given that you are improving this interface, could the Flang alias analysis be replaced with the one from MLIR?
There was a problem hiding this comment.
Flang alias analysis has special handling for HLFIR/FIR operations that carry extra information about the rules of aliasing for objects created from Fortran variables. For example, there are special rules for DUMMY arguments of functions that cannot alias each other unless they have TARGET or POINTER attributes. Flang carries this extra information on [hl]fir.declare operations. For the two given "addresses", Flang alias analysis tries to discover the "sources" of those addresses, e.g. by tracking through view-like operations, and then reasons about the aliasing having reached [hl]fir.declare operations.
I think the HLFIR/FIR specifics used by Flang alias analysis can be abstracted by using some generic interfaces that [hl]fir.declare (and some other operations, e.g. fir.global), but I want to leave this out of scope of this PR.
| MayAlias, | ||
| /// The two locations alias, but only due to a partial overlap. | ||
| /// The two locations overlap in some way, regardless of whether | ||
| /// they start at the same address or not. |
There was a problem hiding this comment.
This seems to change the semantics of PartialAlias. Is it safe?
There was a problem hiding this comment.
I think the new comment is a clarification of the old one (The two locations alias, but only due to a partial overlap.), which may be interpreted differently by the readers.
Yes, I decided to reuse the LLVM definition here, which seems more verbose to me.
I believe there are no current check for PartialAlias or isPartial in MLIR sources, so the change should be safe in a sense that it should not break any existing code. The change seems good to me also because it is more precise.
| /// The two locations precisely alias each other, meaning that | ||
| /// they always start at exactly the same location. | ||
| /// This result does not imply that the pointers compare equal. | ||
| MustAlias, |
There was a problem hiding this comment.
I think it's worth mentioning as a comment of the enum that these values are taken from LLVM.
There was a problem hiding this comment.
Thanks! I will add a comment.
| /// value. | ||
| static constexpr unsigned maxUnderlyingValueSearchDepth = 10; | ||
|
|
||
| /// Given a value, collect all of the underlying values being addressed. |
There was a problem hiding this comment.
Can you add documentation for the arguments? Here and for all functions who's signature has changed.
There was a problem hiding this comment.
Sure, I will add the comments!
| static void | ||
| collectUnderlyingAddressValues(Value value, unsigned maxDepth, | ||
| DenseMap<Value, bool> &visited, bool maybeOffset, | ||
| SmallVectorImpl<std::pair<Value, bool>> &output); |
There was a problem hiding this comment.
I'm not familiar this function. Why is this change needed?
There was a problem hiding this comment.
I will desribe it in the comments, but here is a short summary.
The alias analysis starts at two pointers and walks back the def-use chains. If there is a view-like operation, the walk is continued for the view-source. Eventually, the alias analysis reaches a set of object definitions (e.g. values for which the further walk is not possible), and then uses these object definitions to match them agains each other. For example, if for the two pointers we collected exactly two object definitions (one for each pointer), and they are the same Values, the alias analysis may think that this is a MustAlias case. This might be not correct, if during the walk we met two different view-like operations and they applied some non-equal offsets to the same object definition, e.g.:
%obj = ...
%view1 = view of %obj with offset 0
%view2 = view of %obj with offset %off
access %view1
access %view2
Consider the following cases for the value of %off:
- If it is statically known to be 0, then we should report
MustAlias. - If it is statically known to be not 0, then we should report
PartialAlias. - If its value is unknown, then we should probably report
MayAlias(meaning that it might be eitherPartialAliasorMustAlias).
My change is not trying to reason about the offset value, and uses the maybeOffset attribute to propagate the information that some offset may be applied to the object definition to produce the value of the pointer for which the alias analysis is being queried.
Ideally, the alias analysis should also take into account the size of the accesses through the two pointers, so that it can reason about aliasing within the same object given the sizes and the accumulated offsets.
There was a problem hiding this comment.
This sounds like a bug in the existing alias analysis to me, irrespective of your change. Am I missing something?
What's the reasoning for this?
|
…lvm#165911) For OpenACC clause ordering, such as maintaining appropriate parent-child relationship ordering, we need to be able to walk references back to their base entities. This introduces the operation interface in the `acc` dialect named `PartialEntityAccessOpInterface` which can be used for this purpose. The interface provides two methods: - `getBaseEntity()`: Returns the base entity being accessed - `isCompleteView()`: Indicates whether the access covers the complete entity to allow this interface to be attached to cases that only conditionally offer a partial view This also adds a utility function `mlir::acc::getBaseEntity()` that uses this interface to retrieve the base entity from a value. This work has some similarities with the ViewLikeOpInterface proposal for FIR: llvm#164020 but it differs in the following ways: - Attached only to operations where we can assume a partial entity access - Includes fir.declare operations due to common block storage associations Tests are included that demonstrate the interface on memref.subview operations, implemented locally in the test since memref operations already have ViewLikeOpInterface for similar purposes.
|
Thank you for the thorough review, @matthias-springer!
I will use the same example as above: Depending on the value of
I would like to suggest using In its current implementation, With my changes to propagate only |
6 participants
I am trying to simplify the implementation of Flang's AliasAnalysis
by using ViewLikeOpInterface for some FIR dialect operations.
Currently, Flang's AliasAnalysis implementation explicitly recognizes
specific set of operations, which is inconvenient.
ViewLikeOpInterface may also be used in other parts of Flang.
This patch adds isKnownToHaveSameStart() method to detect
cases where the input and resulting views may be different
by an offset, in which case FIR alias analysis must not return
MustAlias.New method isKnownToBeCompleteView() returns true iff
the resulting view is a complete view of the input view.
This method will be used by the following patches for OpenACC
support in Flang.
This patch also modifies LocalAliasAnalysis to stop returning
MustAliasin cases where bothMustAliasandPartialAliasare possible - we will return
MayAliasnow, which is conservativelycorrect.