[PATCH] D55775: [Driver] Don't override '-march' when using '-arch x86_64h'
qcolombet added a comment. Should we emit an error if we request x86_64h with an arch older than haswell? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55775/new/ https://reviews.llvm.org/D55775 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions
qcolombet updated this revision to Diff 426555.
qcolombet added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
- includes fixes to clang tests that were missed in the original commit.
If someone knows who we can add from the clang side to double check the tests
update that would be great!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124699/new/
https://reviews.llvm.org/D124699
Files:
clang/test/CodeGen/debug-info-block-vars.c
clang/test/CodeGenObjCXX/nrvo.mm
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
llvm/test/Transforms/DeadArgElim/fct_ptr.ll
Index: llvm/test/Transforms/DeadArgElim/fct_ptr.ll
===
--- /dev/null
+++ llvm/test/Transforms/DeadArgElim/fct_ptr.ll
@@ -0,0 +1,67 @@
+; RUN: opt -S %s -deadargelim -o - | FileCheck %s
+; In that test @internal_fct is used by an instruction
+; we don't know how to rewrite (the comparison that produces
+; %cmp1).
+; Because of that use, we used to bail out on removing the
+; unused arguments for this function.
+; Yet, we should still be able to rewrite the direct calls that are
+; statically known, by replacing the related arguments with undef.
+; This is what we check on the call that produces %res2.
+
+define i32 @call_indirect(i32 (i32, i32, i32)* readnone %fct_ptr, i32 %arg1, i32 %arg2, i32 %arg3) {
+; CHECK-LABEL: @call_indirect(
+; CHECK-NEXT:[[CMP0:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR:%.*]], @external_fct
+; CHECK-NEXT:br i1 [[CMP0]], label [[CALL_EXT:%.*]], label [[CHK2:%.*]]
+; CHECK: call_ext:
+; CHECK-NEXT:[[RES1:%.*]] = tail call i32 @external_fct(i32 undef, i32 [[ARG2:%.*]], i32 undef)
+; CHECK-NEXT:br label [[END:%.*]]
+; CHECK: chk2:
+; CHECK-NEXT:[[CMP1:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR]], @internal_fct
+; CHECK-NEXT:br i1 [[CMP1]], label [[CALL_INT:%.*]], label [[CALL_OTHER:%.*]]
+; CHECK: call_int:
+; CHECK-NEXT:[[RES2:%.*]] = tail call i32 @internal_fct(i32 undef, i32 [[ARG2]], i32 undef)
+; CHECK-NEXT:br label [[END]]
+; CHECK: call_other:
+; CHECK-NEXT:[[RES3:%.*]] = tail call i32 @other_fct(i32 [[ARG2]])
+; CHECK-NEXT:br label [[END]]
+; CHECK: end:
+; CHECK-NEXT:[[FINAL_RES:%.*]] = phi i32 [ [[RES1]], [[CALL_EXT]] ], [ [[RES2]], [[CALL_INT]] ], [ [[RES3]], [[CALL_OTHER]] ]
+; CHECK-NEXT:ret i32 [[FINAL_RES]]
+;
+ %cmp0 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @external_fct
+ br i1 %cmp0, label %call_ext, label %chk2
+
+call_ext:
+ %res1 = tail call i32 @external_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+chk2:
+ %cmp1 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @internal_fct
+ br i1 %cmp1, label %call_int, label %call_other
+
+call_int:
+ %res2 = tail call i32 @internal_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+call_other:
+ %res3 = tail call i32 @other_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+end:
+ %final_res = phi i32 [%res1, %call_ext], [%res2, %call_int], [%res3, %call_other]
+ ret i32 %final_res
+}
+
+
+define i32 @external_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
+define internal i32 @internal_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
+define internal i32 @other_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
Index: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
===
--- llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -268,9 +268,12 @@
if (!Fn.hasExactDefinition())
return false;
- // Functions with local linkage should already have been handled, except the
- // fragile (variadic) ones which we can improve here.
- if (Fn.hasLocalLinkage() && !Fn.getFunctionType()->isVarArg())
+ // Functions with local linkage should already have been handled, except if
+ // they are fully alive (e.g., called indirectly) and except for the fragile
+ // (variadic) ones. In these cases, we may still be able to improve their
+ // statically known call sites.
+ if ((Fn.hasLocalLinkage() && !LiveFunctions.count(&Fn)) &&
+ !Fn.getFunctionType()->isVarArg())
return false;
// Don't touch naked functions. The assembly might be using an argument, or
Index: clang/test/CodeGenObjCXX/nrvo.mm
===
--- clang/test/CodeGenObjCXX/nrvo.mm
+++ clang/test/CodeGenObjCXX/nrvo.mm
@@ -22,7 +22,11 @@
X blocksNRVO() {
return ^{
-// CHECK-LABEL: define internal void @___Z10blocksNRVOv_block_invoke
+// With the optimizer enabled, the DeadArgElim pass is able to
+// mark the block litteral address argument as unused and later the
+// related block_litteral global variable is removed.
+// This allows to promote this call to a fas
[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions
qcolombet added a subscriber: vitalybuka. qcolombet added a comment. Hi @thakis , @dyung , @vitalybuka, Thanks for the heads-up and the revert. Fixed clang tests included in the diff. Cheers, -Quentin Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124699/new/ https://reviews.llvm.org/D124699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions
qcolombet added a comment. Thanks @thakis ! @fhahn are you okay with the clang tests update as well? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124699/new/ https://reviews.llvm.org/D124699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9766fed9c10e: [DeadArgElim] Re-apply: Set unused arguments
for internal functions (authored by qcolombet).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124699/new/
https://reviews.llvm.org/D124699
Files:
clang/test/CodeGen/debug-info-block-vars.c
clang/test/CodeGenObjCXX/nrvo.mm
llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
llvm/test/Transforms/DeadArgElim/fct_ptr.ll
Index: llvm/test/Transforms/DeadArgElim/fct_ptr.ll
===
--- /dev/null
+++ llvm/test/Transforms/DeadArgElim/fct_ptr.ll
@@ -0,0 +1,67 @@
+; RUN: opt -S %s -deadargelim -o - | FileCheck %s
+; In that test @internal_fct is used by an instruction
+; we don't know how to rewrite (the comparison that produces
+; %cmp1).
+; Because of that use, we used to bail out on removing the
+; unused arguments for this function.
+; Yet, we should still be able to rewrite the direct calls that are
+; statically known, by replacing the related arguments with undef.
+; This is what we check on the call that produces %res2.
+
+define i32 @call_indirect(i32 (i32, i32, i32)* readnone %fct_ptr, i32 %arg1, i32 %arg2, i32 %arg3) {
+; CHECK-LABEL: @call_indirect(
+; CHECK-NEXT:[[CMP0:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR:%.*]], @external_fct
+; CHECK-NEXT:br i1 [[CMP0]], label [[CALL_EXT:%.*]], label [[CHK2:%.*]]
+; CHECK: call_ext:
+; CHECK-NEXT:[[RES1:%.*]] = tail call i32 @external_fct(i32 undef, i32 [[ARG2:%.*]], i32 undef)
+; CHECK-NEXT:br label [[END:%.*]]
+; CHECK: chk2:
+; CHECK-NEXT:[[CMP1:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR]], @internal_fct
+; CHECK-NEXT:br i1 [[CMP1]], label [[CALL_INT:%.*]], label [[CALL_OTHER:%.*]]
+; CHECK: call_int:
+; CHECK-NEXT:[[RES2:%.*]] = tail call i32 @internal_fct(i32 undef, i32 [[ARG2]], i32 undef)
+; CHECK-NEXT:br label [[END]]
+; CHECK: call_other:
+; CHECK-NEXT:[[RES3:%.*]] = tail call i32 @other_fct(i32 [[ARG2]])
+; CHECK-NEXT:br label [[END]]
+; CHECK: end:
+; CHECK-NEXT:[[FINAL_RES:%.*]] = phi i32 [ [[RES1]], [[CALL_EXT]] ], [ [[RES2]], [[CALL_INT]] ], [ [[RES3]], [[CALL_OTHER]] ]
+; CHECK-NEXT:ret i32 [[FINAL_RES]]
+;
+ %cmp0 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @external_fct
+ br i1 %cmp0, label %call_ext, label %chk2
+
+call_ext:
+ %res1 = tail call i32 @external_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+chk2:
+ %cmp1 = icmp eq i32 (i32, i32, i32)* %fct_ptr, @internal_fct
+ br i1 %cmp1, label %call_int, label %call_other
+
+call_int:
+ %res2 = tail call i32 @internal_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+call_other:
+ %res3 = tail call i32 @other_fct(i32 %arg1, i32 %arg2, i32 %arg3)
+ br label %end
+
+end:
+ %final_res = phi i32 [%res1, %call_ext], [%res2, %call_int], [%res3, %call_other]
+ ret i32 %final_res
+}
+
+
+define i32 @external_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
+define internal i32 @internal_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
+define internal i32 @other_fct(i32 %unused_arg1, i32 %arg2, i32 %unused_arg3) {
+ ret i32 %arg2
+}
+
Index: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
===
--- llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -268,9 +268,12 @@
if (!Fn.hasExactDefinition())
return false;
- // Functions with local linkage should already have been handled, except the
- // fragile (variadic) ones which we can improve here.
- if (Fn.hasLocalLinkage() && !Fn.getFunctionType()->isVarArg())
+ // Functions with local linkage should already have been handled, except if
+ // they are fully alive (e.g., called indirectly) and except for the fragile
+ // (variadic) ones. In these cases, we may still be able to improve their
+ // statically known call sites.
+ if ((Fn.hasLocalLinkage() && !LiveFunctions.count(&Fn)) &&
+ !Fn.getFunctionType()->isVarArg())
return false;
// Don't touch naked functions. The assembly might be using an argument, or
Index: clang/test/CodeGenObjCXX/nrvo.mm
===
--- clang/test/CodeGenObjCXX/nrvo.mm
+++ clang/test/CodeGenObjCXX/nrvo.mm
@@ -22,7 +22,11 @@
X blocksNRVO() {
return ^{
-// CHECK-LABEL: define internal void @___Z10blocksNRVOv_block_invoke
+// With the optimizer enabled, the DeadArgElim pass is able to
+// mark the block litteral address argument as unused and later the
+// related block_litteral global variable is removed.
+// This allows to promote this call to a fastcc call.
+// CHECK-LABEL: define internal fastcc void @___Z10blocks
[PATCH] D103928: [IR] make -warn-frame-size into a module attr
qcolombet accepted this revision. qcolombet added a comment. This revision is now accepted and ready to land. Hi Nick, From the backend prospective, this looks fine but you'll want someone to look at the front end part before landing that change. Cheers, -Quentin Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103928/new/ https://reviews.llvm.org/D103928 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions
qcolombet added a comment. > But that's not a problem of this patch, and we will address that later if > needed. Thanks for looking into it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124699/new/ https://reviews.llvm.org/D124699 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69868: Allow "callbr" to return non-void values
qcolombet added a comment. > I've been concerned about the register live-ins too (I'm less concerned about > the successors issue). Is there documentation on the original decision to > disallow physical register live-ins for MBBs before register allocation? We > could then check to see if we're violating the original reasoning. IIRC the regallocators don't rely on this. Actually, I think the live-ins sets are supposed to be correct only after regalloc (expect for the entry block, I don’t know for the landing pads.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69868/new/ https://reviews.llvm.org/D69868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42276: [Driver] Add an -fexperimental-isel driver option to enable/disable GlobalISel
qcolombet added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:4699
+else
+ CmdArgs.push_back("-global-isel=0");
+ }
I think that it would be useful to also set -global-isel-abort=2, so that users
can report problem early.
What do you think?
Comment at: lib/Driver/ToolChains/Clang.cpp:4699
+else
+ CmdArgs.push_back("-global-isel=0");
+ }
qcolombet wrote:
> I think that it would be useful to also set -global-isel-abort=2, so that
> users can report problem early.
>
> What do you think?
>
>
Should we have some kind of "target validation"?
What I'd like to avoid is people trying the new allocator on unsupported target
(as in the GISel base classes are not present) that would just crash the
compiler.
Alternatively, we could fix the backend to fallback gracefully/abort properly
in those situation.
Right now I believe we would get a segfault on RegisterBankInfo or something
along those lines.
Repository:
rC Clang
https://reviews.llvm.org/D42276
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42276: [Driver] Add an -fexperimental-isel driver option to enable/disable GlobalISel
qcolombet added inline comments.
Comment at: lib/Driver/ToolChains/Clang.cpp:4699
+else
+ CmdArgs.push_back("-global-isel=0");
+ }
aemerson wrote:
> qcolombet wrote:
> > qcolombet wrote:
> > > I think that it would be useful to also set -global-isel-abort=2, so that
> > > users can report problem early.
> > >
> > > What do you think?
> > >
> > >
> > Should we have some kind of "target validation"?
> > What I'd like to avoid is people trying the new allocator on unsupported
> > target (as in the GISel base classes are not present) that would just crash
> > the compiler.
> >
> > Alternatively, we could fix the backend to fallback gracefully/abort
> > properly in those situation.
> > Right now I believe we would get a segfault on RegisterBankInfo or
> > something along those lines.
> > I think that it would be useful to also set -global-isel-abort=2, so that
> > users can report problem early.
> >
> > What do you think?
>
> Yes that makes sense, although should we ignore it for ARM64 O0 since we will
> officially support it?
>
> >
> > Should we have some kind of "target validation"?
> > What I'd like to avoid is people trying the new allocator on unsupported
> > target (as in the GISel base classes are not present) that would just crash
> > the compiler.
>
> Perhaps a warning like "GlobalISel support is incomplete for [target]"? I
> don't know the GISel status for other targets.
>
>
> Yes that makes sense, although should we ignore it for ARM64 O0 since we will
> officially support it?
Sounds sensible. We can advertise the remark option if people want to know what
is going on.
Repository:
rC Clang
https://reviews.llvm.org/D42276
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42276: [Driver] Add an -fexperimental-isel driver option to enable/disable GlobalISel
qcolombet accepted this revision. qcolombet added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D42276 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42860: [ReleaseNotes] Add note for the new -fexperimental-isel flag.
qcolombet accepted this revision. qcolombet added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D42860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
