Skip to content

[Inliner] Propagate more attributes to params when inlining #91101

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 3 commits into from
Oct 16, 2024
Merged

[Inliner] Propagate more attributes to params when inlining #91101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98dd7c6
[Inliner] Add tests for propagating more parameter attributes; NFC
goldsteinn May 4, 2024
b8bd0f1
[Inliner] Propagate more attributes to params when inlining
goldsteinn May 4, 2024
36d5ca0
Index->ArgNo
goldsteinn Oct 14, 2024
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
2 changes: 1 addition & 1 deletion clang/test/CodeGen/attr-counted-by-pr88931.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ void init(void * __attribute__((pass_dynamic_object_size(0))));
// CHECK-LABEL: define dso_local void @_ZN3foo3barC1Ev(
// CHECK-SAME: ptr noundef nonnull align 4 dereferenceable(1) [[THIS:%.*]]) unnamed_addr #[[ATTR0:[0-9]+]] align 2 {
// CHECK-NEXT: entry:
// CHECK-NEXT: tail call void @_Z4initPvU25pass_dynamic_object_size0(ptr noundef nonnull [[THIS]], i64 noundef -1) #[[ATTR2:[0-9]+]]
// CHECK-NEXT: tail call void @_Z4initPvU25pass_dynamic_object_size0(ptr noundef nonnull align 4 dereferenceable(1) [[THIS]], i64 noundef -1) #[[ATTR2:[0-9]+]]
// CHECK-NEXT: ret void
//
foo::bar::bar() {
Expand Down
2 changes: 1 addition & 1 deletion clang/test/OpenMP/bug57757.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ void foo() {
// CHECK-NEXT: ]
// CHECK: .untied.jmp..i:
// CHECK-NEXT: store i32 1, ptr [[TMP2]], align 4, !tbaa [[TBAA16]], !alias.scope [[META13]], !noalias [[META17]]
// CHECK-NEXT: [[TMP4:%.*]] = tail call i32 @__kmpc_omp_task(ptr nonnull @[[GLOB1]], i32 [[TMP0]], ptr [[TMP1]]), !noalias [[META13]]
// CHECK-NEXT: [[TMP4:%.*]] = tail call i32 @__kmpc_omp_task(ptr nonnull @[[GLOB1]], i32 [[TMP0]], ptr nonnull [[TMP1]]), !noalias [[META13]]
// CHECK-NEXT: br label [[DOTOMP_OUTLINED__EXIT]]
// CHECK: .untied.next..i:
// CHECK-NEXT: [[TMP5:%.*]] = getelementptr inbounds nuw i8, ptr [[TMP1]], i64 40
Expand Down
7 changes: 7 additions & 0 deletions llvm/include/llvm/IR/Attributes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,9 @@ class AttributeList {
/// arg.
uint64_t getParamDereferenceableOrNullBytes(unsigned ArgNo) const;

/// Get range (or std::nullopt if unknown) of an arg.
std::optional<ConstantRange> getParamRange(unsigned ArgNo) const;

/// Get the disallowed floating-point classes of the return value.
FPClassTest getRetNoFPClass() const;

Expand Down Expand Up @@ -1123,6 +1126,10 @@ class AttrBuilder {
/// invalid if the Kind is not present in the builder.
Attribute getAttribute(StringRef Kind) const;

/// Retrieve the range if the attribute exists (std::nullopt is returned
/// otherwise).
std::optional<ConstantRange> getRange() const;

/// Return raw (possibly packed/encoded) value of integer attribute or
/// std::nullopt if not set.
std::optional<uint64_t> getRawIntAttr(Attribute::AttrKind Kind) const;
Expand Down
15 changes: 15 additions & 0 deletions llvm/lib/IR/Attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1931,6 +1931,14 @@ AttributeList::getParamDereferenceableOrNullBytes(unsigned Index) const {
return getParamAttrs(Index).getDereferenceableOrNullBytes();
}

std::optional<ConstantRange>
AttributeList::getParamRange(unsigned ArgNo) const {
auto RangeAttr = getParamAttrs(ArgNo).getAttribute(Attribute::Range);
if (RangeAttr.isValid())
return RangeAttr.getRange();
return std::nullopt;
}

FPClassTest AttributeList::getRetNoFPClass() const {
return getRetAttrs().getNoFPClass();
}
Expand Down Expand Up @@ -2277,6 +2285,13 @@ Attribute AttrBuilder::getAttribute(StringRef A) const {
return {};
}

std::optional<ConstantRange> AttrBuilder::getRange() const {
const Attribute RangeAttr = getAttribute(Attribute::Range);
if (RangeAttr.isValid())
return RangeAttr.getRange();
return std::nullopt;
}

bool AttrBuilder::contains(Attribute::AttrKind A) const {
return getAttribute(A).isValid();
}
Expand Down
90 changes: 74 additions & 16 deletions llvm/lib/Transforms/Utils/InlineFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "llvm/Analysis/VectorUtils.h"
#include "llvm/IR/Argument.h"
#include "llvm/IR/AttributeMask.h"
#include "llvm/IR/Attributes.h"
#include "llvm/IR/BasicBlock.h"
#include "llvm/IR/CFG.h"
#include "llvm/IR/Constant.h"
Expand All @@ -59,6 +60,7 @@
#include "llvm/IR/MDBuilder.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/PatternMatch.h"
#include "llvm/IR/ProfDataUtils.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/User.h"
Expand Down Expand Up @@ -1358,18 +1360,36 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,
auto &Context = CalledFunction->getContext();

// Collect valid attributes for all params.
SmallVector<AttrBuilder> ValidParamAttrs;
SmallVector<AttrBuilder> ValidObjParamAttrs, ValidExactParamAttrs;
bool HasAttrToPropagate = false;

// Attributes we can only propagate if the exact parameter is forwarded.
// We can propagate both poison generating and UB generating attributes
// without any extra checks. The only attribute that is tricky to propagate
// is `noundef` (skipped for now) as that can create new UB where previous
// behavior was just using a poison value.
static const Attribute::AttrKind ExactAttrsToPropagate[] = {
Attribute::Dereferenceable, Attribute::DereferenceableOrNull,
Attribute::NonNull, Attribute::Alignment, Attribute::Range};

for (unsigned I = 0, E = CB.arg_size(); I < E; ++I) {
ValidParamAttrs.emplace_back(AttrBuilder{CB.getContext()});
ValidObjParamAttrs.emplace_back(AttrBuilder{CB.getContext()});
ValidExactParamAttrs.emplace_back(AttrBuilder{CB.getContext()});
// Access attributes can be propagated to any param with the same underlying
// object as the argument.
if (CB.paramHasAttr(I, Attribute::ReadNone))
ValidParamAttrs.back().addAttribute(Attribute::ReadNone);
ValidObjParamAttrs.back().addAttribute(Attribute::ReadNone);
if (CB.paramHasAttr(I, Attribute::ReadOnly))
ValidParamAttrs.back().addAttribute(Attribute::ReadOnly);
HasAttrToPropagate |= ValidParamAttrs.back().hasAttributes();
ValidObjParamAttrs.back().addAttribute(Attribute::ReadOnly);

for (Attribute::AttrKind AK : ExactAttrsToPropagate) {
Attribute Attr = CB.getParamAttr(I, AK);
if (Attr.isValid())
ValidExactParamAttrs.back().addAttribute(Attr);
}

HasAttrToPropagate |= ValidObjParamAttrs.back().hasAttributes();
HasAttrToPropagate |= ValidExactParamAttrs.back().hasAttributes();
}

// Won't be able to propagate anything.
Expand All @@ -1391,22 +1411,60 @@ static void AddParamAndFnBasicAttributes(const CallBase &CB,

AttributeList AL = NewInnerCB->getAttributes();
for (unsigned I = 0, E = InnerCB->arg_size(); I < E; ++I) {
// Check if the underlying value for the parameter is an argument.
const Value *UnderlyingV =
getUnderlyingObject(InnerCB->getArgOperand(I));
const Argument *Arg = dyn_cast<Argument>(UnderlyingV);
if (!Arg)
// It's unsound or requires special handling to propagate
// attributes to byval arguments. Even if CalledFunction
// doesn't e.g. write to the argument (readonly), the call to
// NewInnerCB may write to its by-value copy.
if (NewInnerCB->paramHasAttr(I, Attribute::ByVal))
continue;

if (NewInnerCB->paramHasAttr(I, Attribute::ByVal))
// It's unsound to propagate memory attributes to byval arguments.
// Even if CalledFunction doesn't e.g. write to the argument,
// the call to NewInnerCB may write to its by-value copy.
// Don't bother propagating attrs to constants.
if (match(NewInnerCB->getArgOperand(I),
llvm::PatternMatch::m_ImmConstant()))
continue;

unsigned ArgNo = Arg->getArgNo();
// Check if the underlying value for the parameter is an argument.
const Argument *Arg = dyn_cast<Argument>(InnerCB->getArgOperand(I));
unsigned ArgNo;
if (Arg) {
ArgNo = Arg->getArgNo();
// For dereferenceable, dereferenceable_or_null, align, etc...
// we don't want to propagate if the existing param has the same
// attribute with "better" constraints. So remove from the
// new AL if the region of the existing param is larger than
// what we can propagate.
AttrBuilder NewAB{
Context, AttributeSet::get(Context, ValidExactParamAttrs[ArgNo])};
if (AL.getParamDereferenceableBytes(I) >
NewAB.getDereferenceableBytes())
NewAB.removeAttribute(Attribute::Dereferenceable);
if (AL.getParamDereferenceableOrNullBytes(I) >
NewAB.getDereferenceableOrNullBytes())
NewAB.removeAttribute(Attribute::DereferenceableOrNull);
if (AL.getParamAlignment(I).valueOrOne() >
NewAB.getAlignment().valueOrOne())
NewAB.removeAttribute(Attribute::Alignment);
if (auto ExistingRange = AL.getParamRange(I)) {
if (auto NewRange = NewAB.getRange()) {
ConstantRange CombinedRange =
ExistingRange->intersectWith(*NewRange);
NewAB.removeAttribute(Attribute::Range);
NewAB.addRangeAttr(CombinedRange);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, looking at this code again, shouldn't this be querying the attributes on the CallBase instead of the AttributeList? For example, if you have an existing dereferenceable(8) on the function definition and nothing on the call-site, then this could infer dereferenceable(4) at the call-site, which would take precedence over the definition. Though now that I check the implementation of getParamDereferenceableBytes(), it doesn't actually implement the usual fallback to inspecting the attributes on the callee. So for now checking AL is probably fine...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I think this actually implies that we have a bug with out byval handling and should be fixed.

Take:
https://godbolt.org/z/Wj9na57nn

we are propagating readonly to byval, but not picking up byval because its only checking the callbase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will post a fix later tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than the bug with ByVal, I would say that in InstCombine or something we should remove redundant attrs from callbases rather than require yet more special case handling for callbase attrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to merge this (assuming no objections to the above comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, what is your opposition to just cleaning it up explicitly in one place as opposed to having to manage the special cases everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Code is expected to query attributes on CallBase, not the underlying attribute list. If everything works on CallBase, and CallBase implements attribute inheritance correctly, there should not be any special cases relative to the current code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed in query, but not in setting. Seems like a generally easier to design to write whatever is correct to callbase attr list, then let some later pass handle optimizing it properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow. You are already querying the existing attributes here, what difference does it make in terms of complexity to query them on CallBase instead of AttributeList?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, as long we write the CallBase access APIs to take the best of whats on the Function and CallBase its probably fine. Ill work on updating.

AL = AL.addParamAttributes(Context, I, NewAB);
} else {
// Check if the underlying value for the parameter is an argument.
const Value *UnderlyingV =
getUnderlyingObject(InnerCB->getArgOperand(I));
Arg = dyn_cast<Argument>(UnderlyingV);
if (!Arg)
continue;
ArgNo = Arg->getArgNo();
}

// If so, propagate its access attributes.
AL = AL.addParamAttributes(Context, I, ValidParamAttrs[ArgNo]);
AL = AL.addParamAttributes(Context, I, ValidObjParamAttrs[ArgNo]);
// We can have conflicting attributes from the inner callsite and
// to-be-inlined callsite. In that case, choose the most
// restrictive.
Expand Down
Loading
Loading