Page MenuHomePhabricator

Rename options for basic block sections to be consistent
ClosedPublic

Authored by tmsriram on Jul 23 2020, 1:36 PM.

Details

Summary

D68049 created options for basic block sections: -fbasic-block-sections=, -funique-basic-block-section-names. Rename options in llc and lld (--lto-) to be consistent. Specifically,

  • Rename basicblock-sections to basic-block-sections
  • Rename unique-bb-section-names to unique-basic-block-section-names

Further, add a test to lld for --lto-basic-block-sections= and --lto-unique-basic-block-section-names.

Diff Detail

Event Timeline

tmsriram created this revision.Jul 23 2020, 1:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added inline comments.Jul 23 2020, 1:38 PM
llvm/test/CodeGen/X86/basic-block-sections-clusters-eh.ll
1

Do these files need renaming?

tmsriram marked an inline comment as done.Jul 23 2020, 1:40 PM
tmsriram added inline comments.
llvm/test/CodeGen/X86/basic-block-sections-clusters-eh.ll
1

Sure, probably cleaner if it is renamed. Will do.

tmsriram updated this revision to Diff 280259.Jul 23 2020, 2:20 PM

Rename test files too.

MaskRay added inline comments.Jul 28 2020, 7:55 AM
lld/test/ELF/lto/basic-block-sections.ll
2 ↗(On Diff #280259)

Delete this line.

4 ↗(On Diff #280259)

-save-temps -> --save-temps

For LLD specific long options, we prefer double-dash forms.

You can just use -o %t

Do you need another RUN line without --lto-unique-basic-block-section-names ?

tmsriram updated this revision to Diff 281800.Jul 29 2020, 10:09 PM
tmsriram marked 2 inline comments as done.

Add extra test without long section names and other nits.

Add extra test without long section names and other nits.

Looks like I messsed up the renaming somehow. Let me fix that.

Add extra test without long section names and other nits.

Looks like I messsed up the renaming somehow. Let me fix that.

Looks fine to me now. My bad.

MaskRay accepted this revision.Jul 29 2020, 10:16 PM

lld/test/ELF/lto/basic-block-sections.ll should be placed in another patch. Adding a new test is unrelated to renaming the whole bunch of test files.

lld/test/ELF/lto/basic-block-sections.ll
5 ↗(On Diff #281800)

--lto-O2 is the default. I wonder if --lto-O0 is significant. If not, please remove it.

This revision is now accepted and ready to land.Jul 29 2020, 10:16 PM
tmsriram marked an inline comment as done.Jul 30 2020, 9:50 AM

lld/test/ELF/lto/basic-block-sections.ll should be placed in another patch. Adding a new test is unrelated to renaming the whole bunch of test files.

Okay, I will separate the test out.

lld/test/ELF/lto/basic-block-sections.ll
5 ↗(On Diff #281800)

Yes, it is intentional so that the basic blocks are not optimized away.

tmsriram marked an inline comment as done.Jul 30 2020, 9:50 AM
This revision was automatically updated to reflect the committed changes.