This is an archive of the discontinued LLVM Phabricator instance.

[Alignment][NFC] MachineMemOperand::getAlign/getBaseAlign
ClosedPublic

Authored by gchatelet on Mar 27 2020, 7:03 AM.

Details

Summary

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

Diff Detail

Event Timeline

gchatelet created this revision.Mar 27 2020, 7:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 7:03 AM
courbet requested changes to this revision.Mar 27 2020, 7:16 AM
courbet added inline comments.
llvm/lib/CodeGen/MachineOperand.cpp
1059

The outer Align looks unnecessary.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
13653–13654

This is not an NFC. Should this be isAligned() ?

13724–13725

ditto

This revision now requires changes to proceed.Mar 27 2020, 7:16 AM
gchatelet marked 4 inline comments as done.Mar 27 2020, 8:01 AM
gchatelet added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
13653–13654

isAligned currently works the other way around, here is the definition

inline bool isAligned(Align Lhs, uint64_t SizeInBytes) {
  return SizeInBytes % Lhs.value() == 0;
}

Rewritten with isAligned, !(MMO->getAlignment()%16 gives isAligned(Align(16), MMO->getAlign().value()), this is not very readable.

Now !(A % 16) means that A should be a multiple of 16.
Since A is a power of two by definition it is a multiple of 16 iff it is >=16.

So although it is not a straightforward it is NFC.

13724–13725

same here

courbet accepted this revision.Mar 27 2020, 8:16 AM

Thanks for the explanation.

This revision is now accepted and ready to land.Mar 27 2020, 8:16 AM
gchatelet marked 3 inline comments as done.Mar 27 2020, 8:47 AM
gchatelet updated this revision to Diff 253135.Mar 27 2020, 8:47 AM
  • Remove redundant Align
This revision was automatically updated to reflect the committed changes.