Skip to content

[flang] add tbaa tags to global and direct values #68727

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.

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

Closed
wants to merge 16 commits into from

Conversation

tblah
Copy link
Contributor

@tblah tblah commented Oct 10, 2023

The commit to be reviewed here is ba1bbbe (and any fixups). The others are from previous "stacked PRs", the previous one is #68595

These turn out to be useful for spec2017/fotonik3d and are safe so long as
they are not used along side TBAA tags for local allocations. LLVM may
be able to figure out local allocations by itself anyway.

tblah added 15 commits October 5, 2023 14:57
This interface allows (HL)FIR passes to add TBAA information to fir.load
and fir.store. If present, these TBAA tags take precedence over those
added during CodeGen.

We can't reuse mlir::LLVMIR::AliasAnalysisOpInterface because that uses
the mlir::LLVMIR namespace so it tries to define methods for fir
operations in the wrong namespace. But I did re-use the tbaa tag type to
minimise boilerplate code.

The new builders are to preserve the old interface without the tbaa tag.
See RFC at
https://discourse.llvm.org/t/rfc-propagate-fir-alias-analysis-information-using-tbaa/73755

This pass adds TBAA tags to all accesses to non-pointer/target dummy
arguments. These TBAA tags tell LLVM that these accesses cannot alias:
allowing better dead code elimination, hoisting out of loops, and
vectorization.

Each function has its own TBAA tree so that accesses between funtions
MayAlias after inlining.

I also included code for adding tags for local allocations and for
global variables. Enabling all three kinds of tag is known to produce a
miscompile and so these are disabled by default. But it isn't much
code and I thought it could be interesting to play with these later if
one is looking at a benchmark which looks like it would benefit from
more alias information. I'm open to removing this code too.

TBAA tags are also added separately by TBAABuilder during CodeGen.
TBAABuilder has to run during CodeGen because it adds tags to box
accesses, many of which are implicit in FIR. This pass cannot (easily)
run in CodeGen because fir::AliasAnalysis has difficulty tracing values
between blocks, and by the time CodeGen runs, structured control flow
has already been lowered.

Coming in follow up patches
  - Change CodeGen/TBAABuilder to use TBAAForest to add tags within the
    same per-function trees as are used here (delayed to a later patch
    to make it easier to revert)
  - Command line argument processing to actually enable the pass
This is important to ensure that tags end up in the same trees that were
created in the FIR TBAA pass. If they are in different trees then
everything in one tree will be assumed to MayAlias with everything in the
other tree. This leads to poor performance.

@vzakhari requested that the old (not-per-function) trees are
maintained so I left the old test intact.
This is fallout from getting argument names from fir.declare operations
instead of fir.bindc attributes.
The ultimate intention is to have this pass enabled by default whenever
we are optimizing for speed. But for now, just add the arguments so this
can be more easily tested.

Previous PR in this series:
llvm#68437
These turn out to be useful for spec2017/fotonik3d and safe so long as
they are not used along side TBAA tags for local allocations. LLVM may
be able to figure out local allocations by itself anyway.
@@ -406,7 +406,7 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v) {
attributes.set(Attribute::Pointer);
}

if (type == SourceKind::Global)
if (type == SourceKind::Global || type == SourceKind::Direct)
Copy link
Contributor

Choose a reason for hiding this comment

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

We also wanted to use SourceKind::Direct with Allocmem and Alloca in which case global would be undefined.
Maybe we can define the union instead in the TypeSwitch and get rid of this condition.

I am assuming that in tbaa gen, we will be distinguishing based on the SourceKind.
A global SymbolRefAttr with a SourceKind::Global means that we are directly accessing global memory and we know this cannot physically alias with heap and stack.
But a global SymbolRefAttr with a SourceKind::Direct means we do not know much other than the target and pointer attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So SourceKind::Direct isn't necessarily a global?

The use case for the tbaa alias analysis is we want to be able to add tags to global arrays. These can be boxed. The specific case I care about is

module mod
real, dimension(:), allocatable :: glbl

subroutine func(arg)
  real, intent(in), dimension(:) :: arg
  ! glbl can't alias with arg
end subroutine
end module

The generated code looks something like

fir.global @_QMmodEglbl : !fir.box<!fir.heap<!fir.array<?xf32>>> { [...]}

func.func @_QMmodPfunc([...]) {
  [...]
  %glbl_addr = fir.address_of(@_QmodEa) : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
  %decl = fir.declare %glbl_addr [...]
  [...]
  %box = fir.load %decl
  %addr = fir.box_addr %box : (!fir.box<!fir.heap<!fir.array<?xf32>>>) -> !fir.heap<!fir.array<?xf32>>
  [...]
  %element = fir.array_coor %addr(%shape) %index
  %val = fir.array_coor %element

What I want to happen here is to notice that %val comes from a global variable and to tag that load as such. This is a lot clearer from the fortran source than the FIR so maybe we need to store more information somewhere.

As I understand it, you're saying that we can't know much about this variable because the global variable is boxed. Why is the box special?

Copy link
Contributor

Choose a reason for hiding this comment

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

That could be because glbl can be passed as one of the arguments to the subroutine func and hence alias.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am possibly wrong here. Settings of other compilers seem to suggest that globals and dummies do not alias.

from the oracle docs:
-xalias=no%dummy: The Fortran standard is followed and dummy arguments do not alias each other or global variables in the actual call. (This is the default.)

-fargument-noalias-global is the default for gfortran.
https://gcc.gnu.org/onlinedocs/gcc-3.4.6/g77/Aliasing-Assumed-To-Work.html (old page)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, they would not alias unless they have target/pointer attributes. I will be looking into the example and spend more time on this today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your help!

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it help, if I just said that %val does not come from a global variable. It comes from the heap because glbl is an allocatable. The box is global, but the data it wraps is not.

In the example below glbl is a global

real, dimension(1000) :: glbl
contains
subroutine func(arg)
  real, intent(in), dimension(:) :: arg
  call escape(glbl)
end subroutine
end module

In this case the SourceKind is global and we can infer that it cannot alias with anything else because of where it is physically located. But if you prefer to stick to Fortran rules, you can say that it is an F77 construct (since it does not have any target attribute) and therefore does not alias with anything. It would be non-conformant to pass it as an argument.

fir.global @_QMmodEglbl : !fir.array<1000xf32> {
  %0 = fir.zero_bits !fir.array<1000xf32>
  fir.has_value %0 : !fir.array<1000xf32>
}
func.func @_QMmodPfunc(%arg0: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "arg"}) {
  %c1000 = arith.constant 1000 : index
  %0 = fir.address_of(@_QMmodEglbl) : !fir.ref<!fir.array<1000xf32>>
  %1 = fir.shape %c1000 : (index) -> !fir.shape<1>
  %2 = fir.declare %0(%1) {uniq_name = "_QMmodEglbl"} : (!fir.ref<!fir.array<1000xf32>>, !fir.shape<1>) -> !fir.ref<!fir.array<1000xf32>>
  %3 = fir.declare %arg0 {fortran_attrs = #fir.var_attrs<intent_in>, uniq_name = "_QMmodFfuncEarg"} : (!fir.box<!fir.array<?xf32>>) -> !fir.box<!fir.array<?xf32>>
  fir.call @_QPescape(%2) fastmath<contract> : (!fir.ref<!fir.array<1000xf32>>) -> ()
  return
}

In the case of the Direct SourceKind, the outcome w.r.t. arg is the same but the rule is different. We have a direct memory access because, despite the box load and indirect fir.box_addr operations, we could establish that %val is indexing into glbl. There is no target or pointer attribute on arg so we say that it cannot alias with it.

A couple of observations here is:

  1. Direct SourceKind by itself is not sufficient to determine aliasing, the attributes need to be looked at as well.
  2. Unlike the global case, the rule for direct is not reflective. Aliasing is established w.r.t. another value.

Hopefully, this clarifies things. But if none of that helps I am now wondering if we cannot alter the rule to make it reflective. Instead of

lhsSrc.kind == SourceKind::Direct && !rhsSrc.isTargetOrPointer()

could we have

lhsSrc.kind == SourceKind::Direct && !lhsSrc.isTargetOrPointer()

I will play with it today.

Copy link
Contributor Author

@tblah tblah Oct 11, 2023

Choose a reason for hiding this comment

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

So, if I understand correctly, it is safe for me to proceed treating these Direct source kinds as globals (I already handle pointer/target elsewhere)?

The information I am encoding in TBAA is roughly "if it is a pointer, target, or if I don't know what it is then assume it could alias with any other data access". I think this covers both the reflective and non-reflective cases:

  1. rhsSrc.isTargetOrPointer() -> rhs may alias with any data access and so it would alias with lhsSrc no matter what lhsSrc is
  2. lhsSrc.isTargetOrPointer() -> lhs will alias with rhs, no matter what rhs is.

This is less precise than fir::AliasAnalysis::alias but it shouldn't produce incorrect results.

Thanks again for your help on this

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still struggling a bit with heap data being marked global.

What would make sense for me is:

A direct with no pointer/target would have its own TBAA node.
A global with no target would have its own TBBA node.
Anything else would point to the "Any Data Access" node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds good to me. I will update this patch to do that tomorrow. I only added it to the global tree because currently it only seemed to be used to represent global variables and I didn't think much about the heap data.

@tblah tblah changed the title [flang] add tbaa tags to global variables [flang] add tbaa tags to global and direct values Oct 12, 2023
@tblah
Copy link
Contributor Author

tblah commented Oct 24, 2023

@Renaud-K ping

tblah added a commit that referenced this pull request Oct 25, 2023
These turn out to be useful for spec2017/fotonik3d and safe so long as
they are not used along side TBAA tags for local allocations. LLVM may
be able to figure out local allocations by itself anyway.

PR #68727
@tblah
Copy link
Contributor Author

tblah commented Oct 25, 2023

Merged manually in 6242c8c

@tblah tblah closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants