This revision was automatically updated to reflect the committed changes.
Closed by commit rL336717: Patch to fix pragma metadata for do-while loops
(authored by bjope, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
bjope added a comment.
In https://reviews.llvm.org/D48721#1157571, @deepak2427 wrote:
> @Bjorn, Thanks for reviewing and accepting the patch.
>
> Could you please advise on the next steps?
> Would someone else commit this on my behalf or should I request for commit
> access?
>
> Thanks,
>
deepak2427 marked an inline comment as done.
deepak2427 added a comment.
@Bjorn, Thanks for reviewing and accepting the patch.
Could you please advise on the next steps?
Would someone else commit this on my behalf or should I request for commit
access?
Thanks,
Deepak Panickal
bjope accepted this revision.
bjope added a comment.
This revision is now accepted and ready to land.
LGTM!
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bjope added a comment.
In https://reviews.llvm.org/D48721#1153138, @deepak2427 wrote:
> I have updated the test to not run the optimizer. The test I had added
> previously for checking if the unroller is respecting the pragma is useful I
> think. Not sure where that can be added though.
> I
deepak2427 updated this revision to Diff 154244.
deepak2427 added a comment.
Updated with test from Bjorn Pettersson which is much more accurate and
clearer. Thanks!
https://reviews.llvm.org/D48721
Files:
lib/CodeGen/CGStmt.cpp
test/CodeGen/pragma-do-while.cpp
Index:
deepak2427 added a comment.
Yeah, you're right. Only one loop has to be checked in this case. I'll update
the test as per your suggestion. Thank you!
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bjope added inline comments.
Comment at: test/CodeGen/pragma-do-while.cpp:1
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+int test(int a[], int n) {
I think that we can simplify it to use one loop here (as a regression test that
we only put the label
deepak2427 added a comment.
I have updated the test to not run the optimizer. The test I had added
previously for checking if the unroller is respecting the pragma is useful I
think. Not sure where that can be added though.
I guess it's independent of this patch anyway. If the patch and test
deepak2427 updated this revision to Diff 154237.
deepak2427 added a comment.
Update the tests.
https://reviews.llvm.org/D48721
Files:
lib/CodeGen/CGStmt.cpp
test/CodeGen/pragma-do-while.cpp
Index: test/CodeGen/pragma-do-while.cpp
hfinkel added a comment.
In https://reviews.llvm.org/D48721#1152023, @deepak2427 wrote:
> I encountered the issue while working with the unroller and found that it was
> not following the pragma info, and traced it back to the issue with metadata.
> As far as I understood, for for-loops and
hfinkel added a comment.
In https://reviews.llvm.org/D48721#1150677, @bjope wrote:
> In https://reviews.llvm.org/D48721#1150361, @hfinkel wrote:
>
> > In https://reviews.llvm.org/D48721#1150333, @bjope wrote:
> >
> > > Is the fault that the metadata only should be put on the back edge, not
> >
deepak2427 added a comment.
I encountered the issue while working with the unroller and found that it was
not following the pragma info, and traced it back to the issue with metadata.
As far as I understood, for for-loops and while-loops, we add the metadata only
to the loop back-edge. So it
bjope added a comment.
In https://reviews.llvm.org/D48721#1150361, @hfinkel wrote:
> In https://reviews.llvm.org/D48721#1150333, @bjope wrote:
>
> > Is the fault that the metadata only should be put on the back edge, not the
> > branch in the preheader?
>
>
> Yea. Our past thinking has been
hfinkel added a comment.
In https://reviews.llvm.org/D48721#1150333, @bjope wrote:
> I tried running
>
> /clang -cc1 -O3 -funroll-loops -S -emit-llvm pragma-do-while-unroll.cpp -o
> - -mllvm -print-after-all
>
>
>
> and I get this
>
> ...
> !2 = distinct !{!2, !3}
> !3 =
bjope added a comment.
I tried running
/clang -cc1 -O3 -funroll-loops -S -emit-llvm pragma-do-while-unroll.cpp -o -
-mllvm -print-after-all
and I get this
...
!2 = distinct !{!2, !3}
!3 = !{!"llvm.loop.unroll.count", i32 3}
!4 = !{!5, !5, i64 0}
!5 = !{!"int", !6, i64 0}
!6 =
deepak2427 added a comment.
Added to Bugzilla,
https://bugs.llvm.org/show_bug.cgi?id=38011
Repository:
rC Clang
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
deepak2427 added a comment.
Do I need to add specific reviewers?
Repository:
rC Clang
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
deepak2427 added a subscriber: shenhan.
deepak2427 added a comment.
I had based it on the other tests in clang/test/CodeGen.
Do we not need the `-o` to output to standard output?
Or did you mean something else?
Repository:
rC Clang
https://reviews.llvm.org/D48721
I had based it on the other tests in clang/test/CodeGen.
Do we not need the `-o` to output to standard output?
Or did you mean something else?
On Thu, Jun 28, 2018 at 10:25 PM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:
> lebedev.ri added a comment.
>
> I'm not sure we can
lebedev.ri added a comment.
I'm not sure we can use `-O` in tests at all, and i'm not sure it is even
needed here since you are only testing codegen.
Repository:
rC Clang
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
deepak2427 updated this revision to Diff 153393.
deepak2427 added a comment.
Add tests and the patch.
https://reviews.llvm.org/D48721
Files:
lib/CodeGen/CGStmt.cpp
test/CodeGen/pragma-do-while-unroll.cpp
test/CodeGen/pragma-do-while.cpp
Index: test/CodeGen/pragma-do-while.cpp
deepak2427 updated this revision to Diff 153392.
deepak2427 added a comment.
Herald added a subscriber: zzheng.
Add tests.
https://reviews.llvm.org/D48721
Files:
test/CodeGen/pragma-do-while-unroll.cpp
test/CodeGen/pragma-do-while.cpp
Index: test/CodeGen/pragma-do-while.cpp
lebedev.ri added a comment.
In https://reviews.llvm.org/D48721#1146681, @deepak2427 wrote:
> > Phab is the correct way to submit patches.
> > But having a bugreport in bugzilla is good too.
> > But the test will be needed regardless of the patch submission method.
> > And yes, please do
deepak2427 added a comment.
> Phab is the correct way to submit patches.
> But having a bugreport in bugzilla is good too.
> But the test will be needed regardless of the patch submission method.
> And yes, please do always upload all patches with full context (`-U9`).
Sorry about the
>
> Phab is the correct way to submit patches.
> But having a bugreport in bugzilla is good too.
> But the test will be needed regardless of the patch submission method.
> And yes, please do always upload all patches with full context (`-U9`).
Sorry about the context.
Can I add the test file
deepak2427 updated this revision to Diff 153316.
deepak2427 added a comment.
Add full context
https://reviews.llvm.org/D48721
Files:
lib/CodeGen/CGStmt.cpp
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++
lebedev.ri added a comment.
In https://reviews.llvm.org/D48721#1146662, @deepak2427 wrote:
> It's a patch for a bug in clang.
> I have requested for a Bugzilla account, however thought of putting up the
> patch in the meantime.
> Do I need to mark it '[Private]'?
Phab is the correct way to
xbolva00 added a comment.
Please upload patch with full context
Repository:
rC Clang
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
It's a patch for a bug in clang.
I have requested for a Bugzilla account, however thought of putting up the
patch in the meantime.
Do I need to mark it '[Private]'?
On Thu, Jun 28, 2018 at 2:41 PM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:
> lebedev.ri added a comment.
>
>
deepak2427 added a comment.
It's a patch for a bug in clang.
I have requested for a Bugzilla account, however thought of putting up the
patch in the meantime.
Do I need to mark it '[Private]'?
Repository:
rC Clang
https://reviews.llvm.org/D48721
lebedev.ri added a comment.
Test?
(Or was this meant to contain `[Private]` in title?)
Repository:
rC Clang
https://reviews.llvm.org/D48721
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
deepak2427 created this revision.
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D48721
Files:
lib/CodeGen/CGStmt.cpp
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++
33 matches
Mail list logo
