From d7be8227a84f95071911b60d32d751e8780dde12 Mon Sep 17 00:00:00 2001 From: Johannes Doerfert Date: Wed, 11 Jan 2023 17:19:34 -0800 Subject: [PATCH] [Attributor][FIX] Improve care when dealing with liveness This patch adds two checks that have in experiments caused issues. One was an oversight that allowed new AAs during cleanup to be optimistic. The other treated functions as functions even if they were used as values, e.g., in a cast instruction. In such cases we might have assumed the value is dead if the function is not entered, which isn't true. The new test functions don't expose a bug but I kept them around. --- llvm/include/llvm/Transforms/IPO/Attributor.h | 3 +- llvm/lib/Transforms/IPO/Attributor.cpp | 7 ++++ .../IPConstantProp/openmp_parallel_for.ll | 2 +- llvm/test/Transforms/Attributor/align.ll | 46 +++++++++++----------- llvm/test/Transforms/Attributor/value-simplify.ll | 46 ++++++++++++++++++++++ 5 files changed, 80 insertions(+), 24 deletions(-) diff --git a/llvm/include/llvm/Transforms/IPO/Attributor.h b/llvm/include/llvm/Transforms/IPO/Attributor.h index 272d710..41b555b 100644 --- a/llvm/include/llvm/Transforms/IPO/Attributor.h +++ b/llvm/include/llvm/Transforms/IPO/Attributor.h @@ -1596,7 +1596,8 @@ struct Attributor { // If this is queried in the manifest stage, we force the AA to indicate // pessimistic fixpoint immediately. - if (Phase == AttributorPhase::MANIFEST) { + if (Phase == AttributorPhase::MANIFEST || + Phase == AttributorPhase::CLEANUP) { AA.getState().indicatePessimisticFixpoint(); return AA; } diff --git a/llvm/lib/Transforms/IPO/Attributor.cpp b/llvm/lib/Transforms/IPO/Attributor.cpp index 16a1f82..50facdb 100644 --- a/llvm/lib/Transforms/IPO/Attributor.cpp +++ b/llvm/lib/Transforms/IPO/Attributor.cpp @@ -1397,6 +1397,13 @@ bool Attributor::isAssumedDead(const IRPosition &IRP, const AAIsDead *FnLivenessAA, bool &UsedAssumedInformation, bool CheckBBLivenessOnly, DepClassTy DepClass) { + // Don't check liveness for constants, e.g. functions, used as (floating) + // values since the context instruction and such is here meaningless. + if (IRP.getPositionKind() == IRPosition::IRP_FLOAT && + isa(IRP.getAssociatedValue())) { + return false; + } + Instruction *CtxI = IRP.getCtxI(); if (CtxI && isAssumedDead(*CtxI, QueryingAA, FnLivenessAA, UsedAssumedInformation, diff --git a/llvm/test/Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll b/llvm/test/Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll index bd76966..bcfe365 100644 --- a/llvm/test/Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll +++ b/llvm/test/Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll @@ -1,5 +1,5 @@ ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --check-attributes --check-globals -; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=6 -S < %s | FileCheck %s --check-prefixes=CHECK,TUNIT +; RUN: opt -aa-pipeline=basic-aa -passes=attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=16 -S < %s | FileCheck %s --check-prefixes=CHECK,TUNIT ; RUN: opt -aa-pipeline=basic-aa -passes=attributor-cgscc -attributor-manifest-internal -attributor-annotate-decl-cs -S < %s | FileCheck %s --check-prefixes=CHECK,CGSCC ; ; void bar(int, float, double); diff --git a/llvm/test/Transforms/Attributor/align.ll b/llvm/test/Transforms/Attributor/align.ll index 7bda25c..7f54841 100644 --- a/llvm/test/Transforms/Attributor/align.ll +++ b/llvm/test/Transforms/Attributor/align.ll @@ -157,17 +157,19 @@ define internal ptr @f1(ptr readnone %0) local_unnamed_addr #0 { } ; Function Attrs: nounwind readnone ssp uwtable -define internal ptr @f2(ptr readnone %0) local_unnamed_addr #0 { -; CGSCC: Function Attrs: noinline nounwind uwtable -; CGSCC-LABEL: define {{[^@]+}}@f2 -; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR1:[0-9]+]] { -; CGSCC-NEXT: unreachable -; CGSCC: 2: -; CGSCC-NEXT: unreachable -; CGSCC: 3: -; CGSCC-NEXT: unreachable -; CGSCC: 4: -; CGSCC-NEXT: unreachable +define ptr @f2(ptr readnone %0) local_unnamed_addr #0 { +; CHECK: Function Attrs: nofree noinline norecurse nosync nounwind willreturn memory(none) uwtable +; CHECK-LABEL: define {{[^@]+}}@f2 +; CHECK-SAME: (ptr nofree readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR0]] { +; CHECK-NEXT: [[TMP2:%.*]] = icmp eq ptr [[TMP0]], null +; CHECK-NEXT: br i1 [[TMP2]], label [[TMP4:%.*]], label [[TMP3:%.*]] +; CHECK: 3: +; CHECK-NEXT: br label [[TMP5:%.*]] +; CHECK: 4: +; CHECK-NEXT: br label [[TMP5]] +; CHECK: 5: +; CHECK-NEXT: [[TMP6:%.*]] = phi ptr [ [[TMP0]], [[TMP3]] ], [ @a1, [[TMP4]] ] +; CHECK-NEXT: ret ptr [[TMP6]] ; %2 = icmp eq i8* %0, null br i1 %2, label %5, label %3 @@ -188,14 +190,14 @@ define internal ptr @f2(ptr readnone %0) local_unnamed_addr #0 { ; Function Attrs: nounwind readnone ssp uwtable define internal ptr @f3(ptr readnone %0) local_unnamed_addr #0 { -; CGSCC: Function Attrs: noinline nounwind uwtable +; CGSCC: Function Attrs: nofree noinline norecurse nosync nounwind willreturn memory(none) uwtable ; CGSCC-LABEL: define {{[^@]+}}@f3 -; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR1]] { +; CGSCC-SAME: () local_unnamed_addr #[[ATTR0]] { +; CGSCC-NEXT: br label [[TMP2:%.*]] +; CGSCC: 1: ; CGSCC-NEXT: unreachable ; CGSCC: 2: -; CGSCC-NEXT: unreachable -; CGSCC: 3: -; CGSCC-NEXT: unreachable +; CGSCC-NEXT: ret ptr undef ; %2 = icmp eq i8* %0, null br i1 %2, label %3, label %5 @@ -219,7 +221,7 @@ define align 4 ptr @test7() #0 { ; ; CGSCC: Function Attrs: nofree noinline nosync nounwind willreturn memory(none) uwtable ; CGSCC-LABEL: define {{[^@]+}}@test7 -; CGSCC-SAME: () #[[ATTR2:[0-9]+]] { +; CGSCC-SAME: () #[[ATTR1:[0-9]+]] { ; CGSCC-NEXT: [[C:%.*]] = tail call noundef nonnull align 8 dereferenceable(1) ptr @f1() #[[ATTR13:[0-9]+]] ; CGSCC-NEXT: ret ptr [[C]] ; @@ -258,7 +260,7 @@ define internal ptr @f2b(ptr readnone %0) local_unnamed_addr #0 { ; ; CGSCC: Function Attrs: noinline nounwind uwtable ; CGSCC-LABEL: define {{[^@]+}}@f2b -; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR1]] { +; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR2:[0-9]+]] { ; CGSCC-NEXT: unreachable ; CGSCC: 2: ; CGSCC-NEXT: unreachable @@ -289,7 +291,7 @@ define internal ptr @f3b(ptr readnone %0) local_unnamed_addr #0 { ; ; CGSCC: Function Attrs: noinline nounwind uwtable ; CGSCC-LABEL: define {{[^@]+}}@f3b -; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR1]] { +; CGSCC-SAME: (ptr readnone [[TMP0:%.*]]) local_unnamed_addr #[[ATTR2]] { ; CGSCC-NEXT: unreachable ; CGSCC: 2: ; CGSCC-NEXT: unreachable @@ -316,7 +318,7 @@ define align 4 ptr @test7b(ptr align 32 %p) #0 { ; ; CGSCC: Function Attrs: nofree noinline nosync nounwind willreturn memory(none) uwtable ; CGSCC-LABEL: define {{[^@]+}}@test7b -; CGSCC-SAME: (ptr nofree readnone returned align 32 "no-capture-maybe-returned" [[P:%.*]]) #[[ATTR2]] { +; CGSCC-SAME: (ptr nofree readnone returned align 32 "no-capture-maybe-returned" [[P:%.*]]) #[[ATTR1]] { ; CGSCC-NEXT: ret ptr [[P]] ; tail call i8* @f1b(i8* align 8 dereferenceable(1) @a1) @@ -1101,8 +1103,8 @@ attributes #2 = { null_pointer_is_valid } ; TUNIT: attributes #[[ATTR11]] = { nofree nosync nounwind willreturn } ;. ; CGSCC: attributes #[[ATTR0]] = { nofree noinline norecurse nosync nounwind willreturn memory(none) uwtable } -; CGSCC: attributes #[[ATTR1]] = { noinline nounwind uwtable } -; CGSCC: attributes #[[ATTR2]] = { nofree noinline nosync nounwind willreturn memory(none) uwtable } +; CGSCC: attributes #[[ATTR1]] = { nofree noinline nosync nounwind willreturn memory(none) uwtable } +; CGSCC: attributes #[[ATTR2]] = { noinline nounwind uwtable } ; CGSCC: attributes #[[ATTR3]] = { nounwind } ; CGSCC: attributes #[[ATTR4]] = { nofree nosync nounwind } ; CGSCC: attributes #[[ATTR5]] = { nofree norecurse nosync nounwind willreturn memory(argmem: read) } diff --git a/llvm/test/Transforms/Attributor/value-simplify.ll b/llvm/test/Transforms/Attributor/value-simplify.ll index 5471c89..e01a8d8 100644 --- a/llvm/test/Transforms/Attributor/value-simplify.ll +++ b/llvm/test/Transforms/Attributor/value-simplify.ll @@ -1317,6 +1317,52 @@ define internal i32 @ret_speculatable_expr(ptr %mem, i32 %a2) { ret i32 %add } +define internal void @not_called1() { +; CHECK: Function Attrs: nofree norecurse nosync nounwind willreturn memory(none) +; CHECK-LABEL: define {{[^@]+}}@not_called1 +; CHECK-SAME: () #[[ATTR1]] { +; CHECK-NEXT: ret void +; + ret void +} +define internal void @not_called2() { +; CHECK: Function Attrs: nofree norecurse nosync nounwind willreturn memory(none) +; CHECK-LABEL: define {{[^@]+}}@not_called2 +; CHECK-SAME: () #[[ATTR1]] { +; CHECK-NEXT: ret void +; + ret void +} +define internal void @not_called3() { +; CHECK: Function Attrs: nofree norecurse nosync nounwind willreturn memory(none) +; CHECK-LABEL: define {{[^@]+}}@not_called3 +; CHECK-SAME: () #[[ATTR1]] { +; CHECK-NEXT: ret void +; + ret void +} +declare void @useFnDecl(ptr addrspace(42)); +define void @useFnDef(ptr addrspace(42) %arg) { +; CHECK-LABEL: define {{[^@]+}}@useFnDef +; CHECK-SAME: (ptr addrspace(42) [[ARG:%.*]]) { +; CHECK-NEXT: call void @useFnDecl(ptr addrspace(42) [[ARG]]) +; CHECK-NEXT: ret void +; + call void @useFnDecl(ptr addrspace(42) %arg) + ret void +} +define i1 @user_of_not_called() { +; CHECK-LABEL: define {{[^@]+}}@user_of_not_called() { +; CHECK-NEXT: call void @useFnDecl(ptr addrspace(42) noundef addrspacecast (ptr @not_called1 to ptr addrspace(42))) +; CHECK-NEXT: call void @useFnDef(ptr addrspace(42) noundef addrspacecast (ptr @not_called2 to ptr addrspace(42))) +; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr addrspace(42) addrspacecast (ptr @not_called3 to ptr addrspace(42)), null +; CHECK-NEXT: ret i1 [[CMP]] +; + call void @useFnDecl(ptr addrspace(42) addrspacecast (ptr @not_called1 to ptr addrspace(42))) + call void @useFnDef(ptr addrspace(42) addrspacecast (ptr @not_called2 to ptr addrspace(42))) + %cmp = icmp eq ptr addrspace(42) addrspacecast (ptr @not_called3 to ptr addrspace(42)), null + ret i1 %cmp +} ;. ; TUNIT: attributes #[[ATTR0:[0-9]+]] = { nocallback nofree nosync nounwind willreturn } -- 2.7.4