This is an archive of the discontinued LLVM Phabricator instance.

Add -print-schedule scheduling comments to inline asm
ClosedPublic

Authored by avt77 on Nov 7 2017, 4:22 AM.

Details

Summary

Currently we don't generate the scheduler comments for instructions inside inline asm. The patch fixes that and address PR35162.

Diff Detail

Repository
rL LLVM

Event Timeline

avt77 created this revision.Nov 7 2017, 4:22 AM
andreadb edited edge metadata.Nov 8 2017, 9:39 AM

Overall, the patch looks good to me.
I made some minor comments below.

include/llvm/MC/MCParser/MCAsmParser.h
131 ↗(On Diff #121870)

Comments should end with a period. I noticed the same mistake in AsmPrinter.h.

135 ↗(On Diff #121870)

Unrelated to your code review. However, this comment should be associated to field 'HadError' at line 130. I think this should be fixed.

169 ↗(On Diff #121870)

I suggest to rename enablePrintSchedInfo() to shouldPrintSchedInfo().

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
198 ↗(On Diff #121870)

Add a /* unused */ for the bool param.

1046–1047 ↗(On Diff #121870)

I suggest to change EnableSchedInfo to just PrintSchedInfo (or PrintSchedInfoEnabled).

1050–1052 ↗(On Diff #121870)

Same.

lib/Target/X86/AsmParser/X86AsmInstrumentation.h
45–46 ↗(On Diff #121870)

I suggest to change EnableSchedInfo to just PrintSchedInfo (or PrintSchedInfoEnabled).

58–59 ↗(On Diff #121870)

Same.

lib/Target/X86/AsmParser/X86AsmParser.cpp
2704 ↗(On Diff #121870)

I suggest to rename enablePrintSchedInfo() to shouldPrintSchedInfo().

avt77 updated this revision to Diff 122223.EditedNov 9 2017, 3:34 AM

All andreadb's comments were fixed.

andreadb accepted this revision.Nov 9 2017, 4:27 AM

LGTM.

This revision is now accepted and ready to land.Nov 9 2017, 4:27 AM
This revision was automatically updated to reflect the committed changes.