This is an archive of the discontinued LLVM Phabricator instance.

[Pipeliner] Fix the bug in pragma that disables the pipeliner
ClosedPublic

Authored by sgundapa on Mar 17 2020, 10:59 AM.

Details

Diff Detail

Event Timeline

sgundapa created this revision.Mar 17 2020, 10:59 AM
sgundapa set the repository for this revision to rG LLVM Github Monorepo.Mar 17 2020, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 11:01 AM
avl added inline comments.Mar 18 2020, 4:29 AM
llvm/lib/CodeGen/MachinePipeliner.cpp
318

it looks like more appropriate place to set to default state would be in start of setPragmaPipelineOptions:

void MachinePipeliner::setPragmaPipelineOptions(MachineLoop &L) {
  disabledByPragma = false;

Additionally, II_setByPragma could also be set to default state here. though this is for separate patch.

sgundapa updated this revision to Diff 251078.Mar 18 2020, 7:34 AM
avl added inline comments.Mar 18 2020, 7:58 AM
llvm/lib/CodeGen/MachinePipeliner.cpp
298

now it becomes depending on order of metadata. It works for "distinct !{!8, !7, !9}" and does not work for distinct !{!8, !9, !7} . This default value should be set in the first line of setPragmaPipelineOptions (before other instructions):

void MachinePipeliner::setPragmaPipelineOptions(MachineLoop &L) {
  disabledByPragma = false;
sgundapa marked an inline comment as done.Mar 18 2020, 8:13 AM
sgundapa added inline comments.
llvm/lib/CodeGen/MachinePipeliner.cpp
298

I did not realize this is inside the loop. I will move this before we start executing this loop.

sgundapa updated this revision to Diff 251091.Mar 18 2020, 8:15 AM
avl added a comment.Mar 18 2020, 8:29 AM

this lgtm for me, with nit in comments. Also, please wait some time for others opinions.

llvm/lib/CodeGen/MachinePipeliner.cpp
296

please delete this extra line.

avl accepted this revision.Mar 18 2020, 8:29 AM
This revision is now accepted and ready to land.Mar 18 2020, 8:29 AM
sgundapa set the repository for this revision to rG LLVM Github Monorepo.Mar 18 2020, 8:56 AM