This is an archive of the discontinued LLVM Phabricator instance.

[LLVM][Alignment] Make functions using log of alignment explicit
ClosedPublic

Authored by gchatelet on Aug 8 2019, 5:55 AM.

Details

Summary

This patch renames functions that takes or returns alignment as log2, this patch will help with the transition to llvm::Align.
The renaming makes it explicit that we deal with log(alignment) instead of a power of two alignment.
A few renames uncovered dubious assignments

  • MirParser/MirPrinter was expecting powers of two but MachineFunction and MachineBasicBlock were using deal with log2(align). This patch fixes it.
  • MachineBlockPlacement exposes two flags (align-all-blocks and align-all-nofallthru-blocks) supposedly interpreted as power of two alignments, internally these values are interpreted as log2(align). This patch updates the documentation,
  • MachineFunctionexposes exposes align-all-functions also interpreted as power of two alignment, internally this value is interpreted as log2(align). This patch updates the documentation,

Event Timeline

gchatelet created this revision.Aug 8 2019, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 5:55 AM
gchatelet edited the summary of this revision. (Show Details)Aug 8 2019, 5:56 AM
gchatelet edited the summary of this revision. (Show Details)Aug 8 2019, 5:58 AM
gchatelet edited the summary of this revision. (Show Details)Aug 8 2019, 6:02 AM
gchatelet updated this revision to Diff 214128.Aug 8 2019, 6:04 AM
  • Also removes JumpBufSize
gchatelet edited the summary of this revision. (Show Details)Aug 8 2019, 6:05 AM
  • Also removes JumpBufSize
  • Rebasing fix
courbet added inline comments.Aug 22 2019, 2:21 PM
llvm/include/llvm/CodeGen/TargetLowering.h
2689–2690

Maybe split this change to a different commit ? Also, this is part of the public API. There might be external code that depends on this. Not sure what the policy here is. Did you reach out to the original author for this ?

llvm/lib/CodeGen/MachineBlockPlacement.cpp
3066

These are fine, maybe just update the doc of the flags ?

llvm/lib/CodeGen/MachineFunction.cpp
185

ditto.

gchatelet updated this revision to Diff 216958.Aug 23 2019, 2:25 PM
gchatelet marked 4 inline comments as done.
  • Rebasing
  • Update documentation flags.
llvm/include/llvm/CodeGen/TargetLowering.h
2689–2690

This has been submitted as https://reviews.llvm.org/D66683

gchatelet edited the summary of this revision. (Show Details)Aug 23 2019, 2:27 PM

@arphaman do you mind having a look at the MIR parser / writer parts?
How is alignment supposed to be serialized for MIR? As power of two or a log of alignment?

gchatelet updated this revision to Diff 216980.Aug 23 2019, 4:30 PM
  • Rebasing
gchatelet updated this revision to Diff 217001.Aug 23 2019, 9:57 PM
  • Add missing renames
gchatelet edited reviewers, added: arsenm; removed: arphaman.Aug 30 2019, 8:03 AM

@arphaman do you mind having a look at the MIR parser / writer parts?
How is alignment supposed to be serialized for MIR? As power of two or a log of alignment?

Upon closer inspection, it seems like the PowerOfTwo requirement comes from https://github.com/llvm/llvm-project/commit/547a83b4ebd1
@arsenm do you mind having a look ?

gchatelet updated this revision to Diff 218698.Sep 4 2019, 7:38 AM
  • Rebasing
  • Update documentation flags.
  • Add missing renames
  • Use powers of 2 for alignment in YAMLized mir files
  • Make sure YAML MIR has align as Powers of 2. Fix tests.
gchatelet edited the summary of this revision. (Show Details)Sep 4 2019, 7:59 AM
gchatelet added a reviewer: lattner.

@lattner I've added you to this review because I get no feedback from the authors (and you seem to be the OWNER).
The rename exposed a bug in the way alignment of basic blocks and function arguments are serialized for YAML mir.
From this patch and the parseAlignment code it is clear that the serialized form expects powers of two, but the internal structures receiving the alignment deal with log2 of alignments (MachineFunction and MachineBasicBlock).

lattner accepted this revision.Sep 4 2019, 9:29 AM

Renaming this to LogAlignment is much more clear throughout the core infra, thank you! Please get someone who knows the Yaml stuff to approve as well.

This revision is now accepted and ready to land.Sep 4 2019, 9:29 AM

Renaming this to LogAlignment is much more clear throughout the core infra, thank you! Please get someone who knows the Yaml stuff to approve as well.

@arsenm seems to be the one, or maybe @thegameg?

thegameg accepted this revision.Sep 4 2019, 1:10 PM

Thanks for fixing the MIR issues and tests. It would be great to document this somewhere in https://llvm.org/docs/MIRLangRef.html if you can. Other than that, this LGTM!

gchatelet updated this revision to Diff 218868.Sep 5 2019, 2:30 AM
  • Rebasing
  • Update documentation flags.
  • Add missing renames
  • Use powers of 2 for alignment in YAMLized mir files
  • Make sure YAML MIR has align as Powers of 2. Fix tests.
  • Added documentation in MIRLangRef.rst

Thx @thegameg and @lattner , I'll commit shortly!

This revision was automatically updated to reflect the committed changes.