Page MenuHomePhabricator

[Alignment][NFC] Assume AlignmentFromAssumptions::getNewAlignment is always set.
ClosedPublic

Authored by gchatelet on Apr 6 2020, 3:58 AM.

Details

Summary

In D77454 we explain that LoadInst and StoreInst always have their alignment defined.
This allows to work backward here and to infer that getNewAlignment does not need to return 0 in case of failure.
Returning 1 also works since it needs to be greater than the Load/Store alignment which is a least 1.

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.Apr 6 2020, 3:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 3:58 AM

why not change the single return 0 in getNewAlignment() with return Align(1), and modify that type ?

why not change the single return 0 in getNewAlignment() with return Align(1), and modify that type ?

Sure, I wanted to do it in several steps but sounds good to me.

gchatelet updated this revision to Diff 255302.Apr 6 2020, 5:52 AM
  • Address comments
gchatelet marked an inline comment as done.Apr 6 2020, 5:54 AM
gchatelet added inline comments.
llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
182

I could remove these checks since we know both NewAlignment and NewIncAlignment are powers of two (and so are multiples of each other)

courbet added inline comments.Apr 6 2020, 5:58 AM
llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
172

And now getNewAlignmentDiff() can return MaybeAlign, so we do not need that assert.

gchatelet updated this revision to Diff 255309.Apr 6 2020, 6:20 AM
  • Address comments
gchatelet marked an inline comment as done.Apr 6 2020, 6:21 AM
courbet accepted this revision.Apr 6 2020, 7:51 AM
This revision is now accepted and ready to land.Apr 6 2020, 7:51 AM
This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.
llvm/lib/Transforms/Scalar/AlignmentFromAssumptions.cpp
329

Until D77454 lands, you have to use something like "DL.getValueOrABITypeAlignment(LI->getAlign(), LI->getType())", not "*LI->getAlign()". Similar for stores. Yes, it's sort of messy.

Not sure what MemIntrinsic does at the moment. I briefly tried to trace it, I think it crashes if the MemIntrinsic doesn't have an align attribute (which must be non-zero). So I guess that's fine, but we should probably fix the API.