Skip to content

[flang] [NFCI] Using getSource instead of getOriginalDef #128984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 7, 2025
Merged

[flang] [NFCI] Using getSource instead of getOriginalDef #128984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce4bf77
Using getSource instead of getOriginalDef
Renaud-K Feb 27, 2025
4474017
Adding source kind test
Renaud-K Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 22 additions & 60 deletions flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,40 +51,6 @@ static bool hasGlobalOpTargetAttr(mlir::Value v, fir::AddrOfOp op) {
v, fir::GlobalOp::getTargetAttrName(globalOpName));
}

static mlir::Value
getOriginalDef(mlir::Value v,
fir::AliasAnalysis::Source::Attributes &attributes,
bool &isCapturedInInternalProcedure, bool &approximateSource) {
mlir::Operation *defOp;
bool breakFromLoop = false;
while (!breakFromLoop && (defOp = v.getDefiningOp())) {
mlir::Type ty = defOp->getResultTypes()[0];
llvm::TypeSwitch<Operation *>(defOp)
.Case<fir::ConvertOp>([&](fir::ConvertOp op) { v = op.getValue(); })
.Case<fir::DeclareOp, hlfir::DeclareOp>([&](auto op) {
v = op.getMemref();
auto varIf = llvm::cast<fir::FortranVariableOpInterface>(defOp);
attributes |= getAttrsFromVariable(varIf);
isCapturedInInternalProcedure |=
varIf.isCapturedInInternalProcedure();
})
.Case<fir::CoordinateOp>([&](auto op) {
if (fir::AliasAnalysis::isPointerReference(ty))
attributes.set(fir::AliasAnalysis::Attribute::Pointer);
v = op->getOperand(0);
approximateSource = true;
})
.Case<hlfir::DesignateOp>([&](hlfir::DesignateOp op) {
auto varIf = llvm::cast<fir::FortranVariableOpInterface>(defOp);
attributes |= getAttrsFromVariable(varIf);
v = op.getMemref();
approximateSource = true;
})
.Default([&](auto op) { breakFromLoop = true; });
}
return v;
}

static bool isEvaluateInMemoryBlockArg(mlir::Value v) {
if (auto evalInMem = llvm::dyn_cast_or_null<hlfir::EvaluateInMemoryOp>(
v.getParentRegion()->getParentOp()))
Expand Down Expand Up @@ -621,38 +587,34 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
if (mlir::isa<fir::PointerType>(boxTy.getEleTy()))
attributes.set(Attribute::Pointer);

auto def = getOriginalDef(op.getMemref(), attributes,
isCapturedInInternalProcedure,
approximateSource);
if (auto addrOfOp = def.template getDefiningOp<fir::AddrOfOp>()) {
global = addrOfOp.getSymbol();

if (hasGlobalOpTargetAttr(def, addrOfOp))
attributes.set(Attribute::Target);
auto boxSrc = getSource(op.getMemref());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What ensures this getSource call will not recurse through a second LoadOp case? getOriginalDef clearly could not do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the drawback and the reason why I preferred getOrginalDef.
Though, since it is not following data, it should return an indirect source kind and the load op as the source.
I could add a test, I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would also be helpful to have a brief comment on this getSource call: This getSource call will never recurse through a second LoadOp because followData will necessarily become false within the call and thus the above boxTy && followData guard will prevent this block from executing again.

But is that true for the part of the LoadOp case outside that block: the omp::TargetOp handling? Recursing to that from this getSource call was not possible with getOriginalDef, but I'm not sure now.

If we want to be more confident that this PR is NFC, we could add a mode parameter for getSource that causes it to stop with Indirect on any case that getOriginalDef didn't handle. We could then gradually extend the implementation to eliminate those Indirect cases in later PRs as driven by real use cases. That approach seems to achieve a reasonable balance with the maintenance rationale for eliminating getOriginalDef.

attributes |= boxSrc.attributes;
approximateSource |= boxSrc.approximateSource;
isCapturedInInternalProcedure |=
boxSrc.isCapturedInInternalProcedure;

global = llvm::dyn_cast<mlir::SymbolRefAttr>(boxSrc.origin.u);
if (global) {
type = SourceKind::Global;
}
// TODO: Add support to fir.allocmem
else if (auto allocOp =
def.template getDefiningOp<fir::AllocaOp>()) {
v = def;
defOp = v.getDefiningOp();
type = SourceKind::Allocate;
} else if (isDummyArgument(def)) {
defOp = nullptr;
v = def;
} else {
type = SourceKind::Indirect;
auto def = llvm::cast<mlir::Value>(boxSrc.origin.u);
// TODO: Add support to fir.allocmem
if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) {
Comment on lines +601 to +602
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using the boxSrc SourceKind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the attributes are merged and the sourceKind come boxSrc, how much different would it be to just follow the source? :) We would be back to square one.

This is why I want to ultimately introduce SourceKind::Emboxed. It is an indirection for which we are comfortable applying static attributes to make an aliasing determination. While, for other type of indirection, lowering cannot guarantee that an alias situation is always indicated with a fir.ptr type for instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was not suggesting using it in general, I just do not see how if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) + the TODO about allocmem is very different from if (boxSrc.kind == SourceKind::Allocate)

Anyway, as mention below, I share your concern about the propagation of the source kind here, and agree the source kind should either remain Indirect + extra guarantee info (namely, if this is an ALLOCATABLE/POINTER or unknown/compiler generated indirection).

v = def;
defOp = v.getDefiningOp();
type = SourceKind::Allocate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not really get why we can propagate the source kind here. The pointer storage may be local, but the address inside the pointer could very well point to some global or dummy memory.

I know this was already the case, so I am OK with the change, but I think we may need to discuss/clarify this.

Copy link
Contributor Author

@Renaud-K Renaud-K Feb 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised that concern as well. But we do not have a proper way of modeling this as @jdenny-ornl showed here. I do get more meaningful results with the SourceKind::Emboxed here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you concerned specifically for a pointer that is an AllocaOp (locals)? We handle a pointer that is an AddrOfOp (globals) or dummy arg in the same manner: for the address loaded from that pointer, the type is Allocate, Global, or Argument, respectively, even though it might dynamically be the address of some other type.

Previously, we instead set type=Direct for an address loaded from a pointer that is AddrOfOp. I attempted to do the same for dummy args and planned to do the same for AllocaOp, but then we got rid of Direct.

At this point, I haven't tried to figure out whether using Direct would have worked out for all use cases we have discussed since then. Reviewing the rationale for dropping Direct might be worthwhile if you are thinking of introducing another type to serve a role similar to the direction I was heading with Direct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct was a misnomer because you never know what was stored at the address loaded. We can still use aliasing rules on these box indirections because we are comfortable relying on source level attributes (POINTER, TARGET,...). We assume that FIR will not introduce an aliasing situation without these attributes.

FIR does not currently guarantee this for scalars. This distinction only became clear to me recently. So Emboxed would be more precise. If and when we can treat scalars like boxes, we can regroup Emboxed and Indirect and treat them the same.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct was a misnomer

I agree it wasn't the right name for the direction I was taking it.

} else if (isDummyArgument(def)) {
defOp = nullptr;
v = def;
} else {
type = SourceKind::Indirect;
}
}
// TODO: This assignment is redundant but somehow works around an
// apparent MSVC bug reporting "undeclared identifier" at the next
// "breakFromLoop = true;". See
// <https://github.com/llvm/llvm-project/pull/127845#issuecomment-2669829610>.
breakFromLoop = true;
} else {
// No further tracking for addresses loaded from memory for now.
type = SourceKind::Indirect;
return;
}
// No further tracking for addresses loaded from memory for now.
type = SourceKind::Indirect;
breakFromLoop = true;
})
.Case<fir::AddrOfOp>([&](auto op) {
Expand Down
25 changes: 25 additions & 0 deletions flang/test/Analysis/AliasAnalysis/source-kind.fir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// RUN: fir-opt %s -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis))' -debug-only=fir-alias-analysis 2>&1 | FileCheck %s

// CHECK-LABEL: Testing : "_QFPtest"

// Checking that the source kind of a load of a load is SourceKind::Indirect
// CHECK: {test.ptr = "load_load"}
// CHECK-NEXT: SourceKind: Indirect

// Checking that the source kind of a load of an arg is SourceKind::Argument
// CHECK: {test.ptr = "load_arg"}
// CHECK-NEXT: SourceKind: Argument

func.func @_QFPtest(%arg0: !fir.ref<!fir.box<!fir.ptr<f32>>> ) attributes {test.ptr = "func"} {

%0 = fir.alloca !fir.llvm_ptr<!fir.box<!fir.ptr<f32>>>
%1 = fir.convert %arg0 : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> !fir.llvm_ptr<!fir.box<!fir.ptr<f32>>>
fir.store %1 to %0 : !fir.ref<!fir.llvm_ptr<!fir.box<!fir.ptr<f32>>>>
%2 = fir.load %0 : !fir.ref<!fir.llvm_ptr<!fir.box<!fir.ptr<f32>>>>
%3 = fir.convert %2 : (!fir.llvm_ptr<!fir.box<!fir.ptr<f32>>>) -> !fir.ref<!fir.box<!fir.ptr<f32>>>
%15 = fir.load %3 : !fir.ref<!fir.box<!fir.ptr<f32>>>
%16 = fir.box_addr %15 {test.ptr = "load_load"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
%17 = fir.load %arg0 : !fir.ref<!fir.box<!fir.ptr<f32>>>
%18 = fir.box_addr %17 {test.ptr = "load_arg"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32>
return
}