[LLVM][NFC] Adding an Alignment type to LLVM

[gchatelet]

Summary

This patch introduces a type to straighten LLVM's alignment management.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html

The next steps are:

• Use this type throughout LLVM
• Harden the type by setting ALIGN_HARDENED_ALIGNMENT to 1

[gchatelet]

• Moved functions around, added more documentation.

[gchatelet]

• Moved comparison functions outside of the classes.

[gchatelet]

• Fixing typo

[gchatelet]

• Adding missing inline

[jakehehrlich]

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness. I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

[jakehehrlich]

llvm/include/llvm/Support/Alignment.h

69 ↗

(On Diff #210092)

2^32 isn't a value that uint32_t can represent so you might change this. 2^31 is the largest valid power of two, 2^32 - 1 is the largest value that can be represented. I don't think you need to bring negative values in to this since its all using uint32_t. Also my math on this is rusty but I think int32_t(uint32_t(1) << uint32_t(31)) == -1.

94 ↗

(On Diff #210092)

Is halving ever really needed?

101–103 ↗

(On Diff #210092)

Seems like you'd just want to provide helper functions like "alignTo" that do this for you. There's a whole family of them but personally I've primarily used "alignTo" and one other really odd highly specific one for ELF to deal with segment alignment.

107 ↗

(On Diff #210092)

If size is a concern yet we only want to represent powers of two I think encoding the power two only would let us represent 64-bit alignments using only a 1 byte value at the cost of a slight bit of increased computation on each operation. These operations tend to not be very performance critical however.

115 ↗

(On Diff #210092)

What's the use case for this type? I don't think any ELF tools have a use case for this at least. ELF uses 0 to mean the same thing as 1 so we'd just want something that converted to 1 asap.

I think its interface should copy Optional if this has a use case. e.g. using Optional<Align> should function close to the same, perhaps with some operations from Align that act in sensible ways.

157–159 ↗

(On Diff #210092)

I don't think you should be providing this. it's easy enough to use "isSet" and their own assert. If I was reading this I wouldn't expect this to be an assert under the hood and nothing else.

179–181 ↗

(On Diff #210092)

Why add a deprecated method to new code? Nothing is using this so there's no need for backward compatibility. Also the two methods above look equally dangerous but boil down to Optional's 'get' or dereference.

192–194 ↗

(On Diff #210092)

Why does choosing false make sense here? It isn't clear to me what should be returned here one way or the other. If padding is needed then one would assume you'd also need a method to perform the padding required. If this was an empty alignment how could you do that?

210 ↗

(On Diff #210092)

I see a lot of "bogus" or "dangerous" or "deprecated" things being added and these all huge big red flags to me. Can you better justify the need for them?

251 ↗

(On Diff #210092)

I'd probably not use const Align & and instead just use Align. This matches how alignment is used over the code base anyhow things like ArrayRef and StringRef are used.

252 ↗

(On Diff #210092)

You can use (Size + A.value() - 1) & -A.value() since you explicitly only represent powers of two. Not critical by any means but a possible implementation that saves on division operations which can be almost as slow as an allocation.

You can think of what you're doing as being like a right shift followed by a left shift which zeros out the extra bits produced in the bump. -A.value() is a mask where all those bits being zeroed out are just zero. I've found that both myself and many other people get confused by this trick so a comment is worth while if you use it.

262 ↗

(On Diff #210092)

I think this being the default makes sense but it would be worth showing examples of where this is used. This for instance matches what ELF should do with zero alignment but thats really just the same as saying that what MaybeAlignment calls undefined is actully an alignment of 1.

271 ↗

(On Diff #210092)

Is there a reason to 1) copy the naming convention here or 2) to not just call this "Log2"? Also this operation will just be "return value" if you use the 1-byte representation proposed above.

A merit of using the bit shift representation is that the cost of converting to the origional is low (1 << value) but using the current representation Log2_32 is quite complicated under the hood and requires a large handful of bitwise operations.

321 ↗

(On Diff #210092)

1. Does any existing code use this
2. Why add the '1'? Id expect this to be the inverse of 1 << x but instead its the inverse of 1 << (x - 1) or as you do below (1 << x) >> 1

339 ↗

(On Diff #210092)

Would be nice to have a version for ELF specifically that treated '0' and '1' as equal.

373 ↗

(On Diff #210092)

Ditto about ELF specific option. Not sure how other parts of the code treat '0' so this might be a follow up.

441 ↗

(On Diff #210092)

How does Optional<T> handle equality in the None case? I think we should match those semantics....maybe depends on what they are.

501 ↗

(On Diff #210092)

Won't std::max do this automatically since you implemented the comparison operators?

518–528 ↗

(On Diff #210092)

These are again consistent with how ELF treats 0 since 0 <= x for all alignments x in ELF but some motivation for how these are used is required IMO since it isn't a priori clear what these should do.

552–562 ↗

(On Diff #210092)

ditto on justification for these.

llvm/unittests/Support/AlignmentTest.cpp

30–31 ↗

(On Diff #210092)

These values are wrong and not powers of two.

[gchatelet]

In D64790#1588231, @jakehehrlich wrote:

First things first, thx a lot for the review. I really appreciate it.

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness.

Ok

I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

Throughout LLVM we mostly use unsigned to represent an alignment. I don't know for sure what was the rationale for using it instead of uint64_t, it can be for historic reasons (just use the machine's word type) or for space reasons.
A quick estimation via git grep -E "^\s+unsigned [Aa]lign.*" | wc -l gives the following number of occurrences (for the whole llvm-project repo)

• unsigned 200
• uint16_t 4
• uint32_t 132
• uint64_t 44

One of the occurrences for the uint16_t is in llvm/include/llvm/CodeGen/TargetLowering.h so for this one at least is seems that storage is important.

This tends to prove that the Alignment type should be parameterized as you suggest.

[gchatelet]

A few comments for now, I'll reply to the other ones soonish.

llvm/include/llvm/Support/Alignment.h

69 ↗

(On Diff #210092)

Thx for noticing, this comment is nonsense indeed.
-1 is all ones and can't be a power of two so there's no need bothering.

94 ↗

(On Diff #210092)

Yes, all the methods here correspond to real code encountered while refactoring llvm-project.

• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6751
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L16529
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L5823
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMFrameLowering.cpp#L2054
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMISelLowering.cpp#L11753
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/ARM/ARMISelLowering.cpp#L12494

Would you prefer operator/ and operator/=?

Please keep in mind that I didn't try to refactor the original code, the first step was introduce the type without touching too much code. The patch corresponding to the whole change is already 26K lines.

101–103 ↗

(On Diff #210092)

I've encountered this pattern a number of times and considered it was worth factoring.

• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/IR/DataLayout.cpp#L58
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/IR/DataLayout.cpp#L75
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp#L506
• https://github.com/llvm/llvm-project/blob/9eb95903da4575b3b95d1a743d48e51bb5026ccd/llvm/lib/Transforms/Coroutines/CoroFrame.cpp#L329

I agree it doesn't have to be a struct member and can be changed to a free function. Any suggestion for a better name?

107 ↗

(On Diff #210092)

Yes that's a possibility I wanted to explore as well, but I'm not sure about the tradeoffs at this point.
IMHO the code would be much more straightforward compared to a templated version.

I'm interested in comments from other developers here.

Storing alignment as a byte would allow alignment up to 2^254

0:0
1:1
2:2
3:4
4:8
...
255: 2^254

115 ↗

(On Diff #210092)

Yes semantically this is exactly Optional<Align>, the use of this type is pervasive in LLVM where the alignment requirements are first unknown and set down the road when needed.
Problem is llvm::Optional needs an additional bool to know if the value is set or not so MaybeAlign is more compact than Optional<Align> (https://godbolt.org/z/8_uA_D) it's also more customizable.

157–159 ↗

(On Diff #210092)

SGTM will remove

[gchatelet]

llvm/include/llvm/Support/Alignment.h

179–181 ↗

(On Diff #210092)

ALIGN_DEPRECATED is defined only when ALIGN_HARDENED_ALIGNMENT is set.
As of this patch there will be no deprecation message, it's useful as a second step to find fishy code.
Fixing the code while introducing the type is not feasible considering the patch is already humongous.

192–194 ↗

(On Diff #210092)

I concur this does not make sense but this is what the code is doing right now. Since this patch is about introducing a type and not fix the code, I've to add this function for now.
This is why these functions will be deprecated afterwards.

210 ↗

(On Diff #210092)

Same as previous comment, this is actual code that seems to be wrong but not to be fixed in this patch.
e.g. of isAlignedBogus https://github.com/llvm/llvm-project/blob/8b7041a5c6f0a373d4886ca807d89790ad6dedab/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp#L8701

251 ↗

(On Diff #210092)

SGTM

252 ↗

(On Diff #210092)

I'm happy to optimize this afterwards if you don't mind. The review will be easier if I just move code around without changing it.

[jakehehrlich]

In D64790#1588982, @gchatelet wrote:

In D64790#1588231, @jakehehrlich wrote:

First things first, thx a lot for the review. I really appreciate it.

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness.

Ok

I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

Throughout LLVM we mostly use unsigned to represent an alignment. I don't know for sure what was the rationale for using it instead of uint64_t, it can be for historic reasons (just use the machine's word type) or for space reasons.
A quick estimation via git grep -E "^\s+unsigned [Aa]lign.*" | wc -l gives the following number of occurrences (for the whole llvm-project repo)

• unsigned 200
• uint16_t 4
• uint32_t 132
• uint64_t 44

One of the occurrences for the uint16_t is in llvm/include/llvm/CodeGen/TargetLowering.h so for this one at least is seems that storage is important.

This tends to prove that the Alignment type should be parameterized as you suggest.

In those 44 cases and 132 cases includes all ELF code. Also consider lld for more uses. You alienate no one if you support up to 64-bits but you alienate all ELF programmers if you don't support 64-bits. I am one of those people. I cannot use this if 64-bits isn't supported. Other than trying to help llvm code quality improve and being a good citizen I have no reason to review this if I don't get 64-bit support.

llvm/include/llvm/Support/Alignment.h

101–103 ↗

(On Diff #210092)

Those are all easily refactored. I'm not sure how "IsPadded" is being propogated though but if that's needed you can check to see if the value changes. In general needing to check for padding like most of those examples are is anti pattern. The error checking case can just be X != alignTo(X, Align)

107 ↗

(On Diff #210092)

0 would represent an alignment of '1' not '0' but you might take that route in the "maybe" to encode a missing value.

115 ↗

(On Diff #210092)

Right I understand the optimization I'm saying you should match the interface modulo a few operations specific to alignments.

179–181 ↗

(On Diff #210092)

You can just remove all the deprecated methods and then gradually refactor other code. Nothing calls this yet so there's no reason to add deprecated functionality.

192–194 ↗

(On Diff #210092)

You should introduce a new proper type, and fix code as its migrated over, not introduce more broken code and gradually unbreak it.

[gchatelet]

In D64790#1592266, @jakehehrlich wrote:

In D64790#1588982, @gchatelet wrote:

In D64790#1588231, @jakehehrlich wrote:

First things first, thx a lot for the review. I really appreciate it.

Ok so I thought about endianness more and I think that's not an issue because computations are generally done in native endianness.

Ok

I do however think you should definitely have either the 64 bit version be the default or have both a 32 and 64 bit version. This type is most useful in cases where tools like lld, llvm-objcopy, yaml2obj, etc... would be using an internal structure or temporary variable and generally you want to handle both 32-bit and 64-bit operations in these cases. The solution is always to default to the highest bit size which is 64-bits because it will handle the 32-bit case just fine. Unless there's a compelling reason to not use 64-bits I think you should just make it your default. Certainly before lld or llvm-objcopy can use this we need to have a 64-bit version.

Throughout LLVM we mostly use unsigned to represent an alignment. I don't know for sure what was the rationale for using it instead of uint64_t, it can be for historic reasons (just use the machine's word type) or for space reasons.
A quick estimation via git grep -E "^\s+unsigned [Aa]lign.*" | wc -l gives the following number of occurrences (for the whole llvm-project repo)

• unsigned 200
• uint16_t 4
• uint32_t 132
• uint64_t 44

One of the occurrences for the uint16_t is in llvm/include/llvm/CodeGen/TargetLowering.h so for this one at least is seems that storage is important.

This tends to prove that the Alignment type should be parameterized as you suggest.

In those 44 cases and 132 cases includes all ELF code. Also consider lld for more uses. You alienate no one if you support up to 64-bits but you alienate all ELF programmers if you don't support 64-bits. I am one of those people. I cannot use this if 64-bits isn't supported. Other than trying to help llvm code quality improve and being a good citizen I have no reason to review this if I don't get 64-bit support.

Please don't get me wrong, I'm absolutely willing to support 64-bits. Here I'm trying to see what's the best design since there are different tradeoffs:

• Have a single 64-bits type: accommodates all usage but can be a problem where space matters
• Have a templated type so people can use whatever suites them best, this complicates the code though.
• Have a compressed representation in a single byte but paying an extra cost when accessing it.

I don't think I ever said I would not add 64-bits support, if it looks like so I apologize for the misunderstanding.

[gchatelet]

llvm/include/llvm/Support/Alignment.h

101–103 ↗

(On Diff #210092)

SGTM, I'll remove it.

115 ↗

(On Diff #210092)

Acknowledged. Will do.

179–181 ↗

(On Diff #210092)

I understand your reluctance to see these deprecated methods in new code but I've went through the exercise of the whole refactoring. It took me about 4 weeks of full time job. This change is not light, it is massive (patch is 26 000 lines). My decision to go this route was to make the review as straightforward as possible so it has a chance to happen. I also wanted to leave breadcrumbs along the way so fishy code could be fixed as a second step.

Since you seem to feel strongly about this I will remove the deprecated methods but I'm not so sure this refactoring will happen thoroughly considering the required investment.

192–194 ↗

(On Diff #210092)

I don't think it introduces "more broken code" since it factors the broken code in a single place but I understand your position and I will try to follow that path.
I'm worried that the incremental fixing may take forever and that I may be dragged away before finishing it.

262 ↗

(On Diff #210092)

IMHO ELF shouldn't use the MaybeAlign type since it represent something that is not yet defined.
AFAIU ELF always deals with known alignment so it should use Align that has well defined semantics.
I envision an Align assumeAligned(uint64_t) function that will make 0 be a Align(1)

321 ↗

(On Diff #210092)

1

Unfortunately yes, here are a few of them

• https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/CodeGen/MachineOperand.cpp#L1039
• https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp#L2911
• https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/CodeGen/MachineOperand.cpp#L1005
• https://github.com/llvm/llvm-project/blob/fecf43eba3630aeb55c56e5d308f99a7bd05bfbe/llvm/lib/IR/Instructions.cpp#L1247

2

I guess it's because they also want to encode the undefined as 0 as it is common in LLVM.

339 ↗

(On Diff #210092)

I'm not sure about this one, I would encourage using the bool operator==(Align Lhs, Align Rhs) version with the assumeAligned() function I was talking earlier - and not compare directly with 0.

441 ↗

(On Diff #210092)

Here are the definitions: https://llvm.org/doxygen/Optional_8h_source.html
I've tightened the defintions for MaybeAlign compared to Optional: the code will check fail when comparing the None case to a real value.

501 ↗

(On Diff #210092)

Yes I removed them.

llvm/unittests/Support/AlignmentTest.cpp

30–31 ↗

(On Diff #210092)

Got rid of them.

[gchatelet]

• Address most of the comments

[gchatelet]

• Remove old documentation, fix Log2_32 -> Log2_64

[gchatelet]

• Enable asserts since we will now fix the code while introducing the

[gchatelet]

• Add more division tests

[gchatelet]

• Fix include guard

[gchatelet]

A gentle ping

[jfb]

llvm/include/llvm/Support/Alignment.h

21 ↗

(On Diff #211089)

I don't think the prior history is useful here.

73 ↗

(On Diff #211089)

You can remove the ctor and instead do uint8_t ShiftValue = 0; above.

99 ↗

(On Diff #211089)

Why is this better than using MaybeAligned = Optional<Align>?
Do we care about packing that extra bit?
It also seems that Align can have a shift of 255 and this can't.

[gchatelet]

• Make MaybeAlign an llvm::Optional.
• Remove unnecessary documentation,

[gchatelet]

Thx for the review @jfb. Anything else?

llvm/include/llvm/Support/Alignment.h

21 ↗

(On Diff #211089)

Indeed. I removed it.

99 ↗

(On Diff #211089)

When the data type for Align was unsigned or uint64_t, llvm::Optional was costly in terms of storage

sizeof(llvm::Optional<unsigned>) == 8
sizeof(llvm::Optional<uint64_t>) == 16

But with Align using uint8_t I agree it's not a big deal anymore sizeof(llvm::Optional<uint8_t>) == 2

I've added a comment about Align::ShiftValue being less than 64 by construction.

[jfb]

llvm/include/llvm/Support/Alignment.h

40 ↗

(On Diff #212162)

final is kinda weird here. Not that I'd inherit from this... but why have final?

146 ↗

(On Diff #212162)

Are the places where Log2 with undefined alignment is called today? Can we report fatal error instead (or assert)?

[gchatelet]

• Assert when Log2 on undefined MaybeAlign.

llvm/include/llvm/Support/Alignment.h

40 ↗

(On Diff #212162)

It was a way to document that this type is not to be derived from. It does not have a virtual destructor.
Now I don't have a strong opinion and I'm happy to remove it if you think it's too weird.

146 ↗

(On Diff #212162)

Since I wanted to first introduce the type and then fix invalid code I never had a chance to run LLVM with all the asserts.
By looking at the code it is definitely possible but I can't tell for sure if it happens. It's one of the motivation for introducing the type in the first place.

For instance it is used in TargetCallingConv where A may well be 0. In that case Log2_32 will return -1, hence the + 1 to encode invalid as zero.

Now that it's clear we want to fix the code as we introduce the type, we can treat it as an error as you suggest.

[gchatelet]

• Fix documentation

[jfb]

Overall this looks good, thanks!

llvm/include/llvm/Support/Alignment.h

40 ↗

(On Diff #212162)

It's just not something we really do anywhere.

146 ↗

(On Diff #212162)

Thanks, I think this is probably the right approach since it forces the changes as adoption happens.

[gchatelet]

• Remove final from Align and MaybeAlign

[gchatelet]

Thank you for the review @jfb and @jakehehrlich. I'll submit the patch now and start working on the migration.
Let me know if you have additional comments.

llvm/include/llvm/Support/Alignment.h

40 ↗

(On Diff #212162)

I removed the final here and below.

[gchatelet]

This patch broke buildbot, I've submitted to fix MSVC https://reviews.llvm.org/rL367400

[foad]

This also broke ninja check-llvm-unit on a Release build:

Failing Tests (7):
    LLVM-Unit :: Support/./SupportTests/Alignment.CantConvertUnsetMaybe
    LLVM-Unit :: Support/./SupportTests/Alignment.CompareAlignToUndefMaybeAlign
    LLVM-Unit :: Support/./SupportTests/Alignment.CompareMaybeAlignToZero
    LLVM-Unit :: Support/./SupportTests/Alignment.ComparisonsWithZero
    LLVM-Unit :: Support/./SupportTests/Alignment.Division
    LLVM-Unit :: Support/./SupportTests/Alignment.InvalidCTors
    LLVM-Unit :: Support/./SupportTests/Alignment.Log2

Typical failure:

FAIL: LLVM-Unit :: Support/./SupportTests/Alignment.Division (2696 of 3890)
******************** TEST 'LLVM-Unit :: Support/./SupportTests/Alignment.Division' FAILED ********************
Note: Google Test filter = Alignment.Division
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from Alignment
[ RUN      ] Alignment.Division
/home/jayfoad2/git/llvm-project/llvm/unittests/Support/AlignmentTest.cpp:70: Failure
Death test: Align(Value) / 2
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/home/jayfoad2/git/llvm-project/llvm/unittests/Support/AlignmentTest.cpp:71: Failure
Death test: MaybeAlign(Value) / 2
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/home/jayfoad2/git/llvm-project/llvm/unittests/Support/AlignmentTest.cpp:79: Failure
Death test: Align(8) / 0
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/home/jayfoad2/git/llvm-project/llvm/unittests/Support/AlignmentTest.cpp:80: Failure
Death test: Align(8) / 3
    Result: failed to die.
 Error msg:
[  DEATH   ] 
[  FAILED  ] Alignment.Division (2 ms)
[----------] 1 test from Alignment (2 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (2 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Alignment.Division

 1 FAILED TEST

[gchatelet]

In D64790#1608212, @foad wrote:

This also broke ninja check-llvm-unit on a Release build:

Thx for letting me know @foad , I'm on it.

[RKSimon]

@gchatelet This is breaking windows bots: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-win-fast/builds/1933 - please can you take a look? I think we'll need to revert until you have a fix.

[gchatelet]

In D64790#1608236, @RKSimon wrote:

@gchatelet This is breaking windows bots: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-win-fast/builds/1933 - please can you take a look? I think we'll need to revert until you have a fix.

If this is about the missing default constructor it's already fixed:
https://reviews.llvm.org/rL367400

As for the failing tests in release the patch is sent as well
https://reviews.llvm.org/rL367427

[RKSimon]

Thanks - rL367427 seemed to be the key

[aprantl]

llvm/trunk/include/llvm/Support/Alignment.h

39

Can you please doxygen-ify all the comments in this file? I.e. // -> ///

113

This comment also doesn't follow the LLVM rules. It should be just
/// Checks that ...

[gchatelet]

@aprantl I've created https://reviews.llvm.org/D65558 to address your comments since this patch is already submitted.

[jhenderson]

Thanks for looking at this. I haven't looked at this or the related patches at all, and I know this has landed already. I haven't really looked at the details of the patch, but I did want to highlight a possible conflict in interpretation of 0 alignment. In the elf gABI, the two alignment fields (sh_addralign and p_align) both have the statement "Values 0 and 1 mean no alignment is required". In other words, 0 is not technically an undefined alignment any more than 1 is, at least in the ELF context. That being said, if you are always treating an alignment of 0 as an alignment of 1, I don't think there's any issue.

[gchatelet]

In D64790#1667895, @jhenderson wrote:

Thanks for looking at this. I haven't looked at this or the related patches at all, and I know this has landed already. I haven't really looked at the details of the patch, but I did want to highlight a possible conflict in interpretation of 0 alignment. In the elf gABI, the two alignment fields (sh_addralign and p_align) both have the statement "Values 0 and 1 mean no alignment is required". In other words, 0 is not technically an undefined alignment any more than 1 is, at least in the ELF context. That being said, if you are always treating an alignment of 0 as an alignment of 1, I don't think there's any issue.

Thx for the feedback @jhenderson!
I had a quick exchange with @lattner a while ago. He told me that 0 most probably means 1 byte aligned, most of the time, which is inline with what you say.

The type has landed but there's a lot more work to get the codebase converted to use it.
This query shows the patches submitted so far.

I'm actively working on it, along the way I learned that the unsigned sometimes means an alignment in bytes (powers of two) but also sometimes the log2 of it, in which case 0 means 1ULL << 0, i.e. 1 byte aligned (see D65945).
I'm as careful as possible and I make sure I understand the consequences when I introduce the type to a part of the codebase, the whole migration will certainly take time.

llvm/trunk/include/llvm/Support/Alignment.h

+//===-- llvm/Support/Alignment.h - Useful alignment functions ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains types to represent alignments.
+// They are instrumented to guarantee some invariants are preserved and prevent
+// invalid manipulations.
+//
+// - Align represents an alignment in bytes, it is always set and always a valid
+// power of two, its minimum value is 1 which means no alignment requirements.
+//
+// - MaybeAlign is an optional type, it may be undefined or set. When it's set
+// you can get the underlying Align type by using the getValue() method.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_ALIGNMENT_H_
+#define LLVM_SUPPORT_ALIGNMENT_H_
+
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Compiler.h"
+#include "llvm/Support/MathExtras.h"
+#include <cassert>
+#include <limits>
+
+namespace llvm {
+
+#define ALIGN_CHECK_ISPOSITIVE(decl)                                           \
+  assert(decl > 0 && (#decl " should be defined"))
+#define ALIGN_CHECK_ISSET(decl)                                                \
+  assert(decl.hasValue() && (#decl " should be defined"))
+
+// This struct is a compact representation of a valid (non-zero power of two)
+// alignment.
+// It is suitable for use as static global constants.
+struct Align {
+private:
+  uint8_t ShiftValue = 0; // The log2 of the required alignment.
+                          // ShiftValue is less than 64 by construction.
+
+  friend struct MaybeAlign;
+  friend unsigned Log2(Align);
+  friend bool operator==(Align Lhs, Align Rhs);
+  friend bool operator!=(Align Lhs, Align Rhs);
+  friend bool operator<=(Align Lhs, Align Rhs);
+  friend bool operator>=(Align Lhs, Align Rhs);
+  friend bool operator<(Align Lhs, Align Rhs);
+  friend bool operator>(Align Lhs, Align Rhs);
+  friend unsigned encode(struct MaybeAlign A);
+  friend struct MaybeAlign decodeMaybeAlign(unsigned Value);
+
+public:
+  // Default is byte-aligned.
+  Align() = default;
+  // Do not perform checks in case of copy/move construct/assign, because the
+  // checks have been performed when building `Other`.
+  Align(const Align &Other) = default;
+  Align &operator=(const Align &Other) = default;
+  Align(Align &&Other) = default;
+  Align &operator=(Align &&Other) = default;
+
+  explicit Align(uint64_t Value) {
+    assert(Value > 0 && "Value must not be 0");
+    assert(llvm::isPowerOf2_64(Value) && "Alignment is not a power of 2");
+    ShiftValue = Log2_64(Value);
+    assert(ShiftValue < 64 && "Broken invariant");
+  }
+
+  // This is a hole in the type system and should not be abused.
+  // Needed to interact with C for instance.
+  uint64_t value() const { return uint64_t(1) << ShiftValue; }
+};
+
+// Treats the value 0 as a 1, so Align is always at least 1.
+inline Align assumeAligned(uint64_t Value) {
+  return Value ? Align(Value) : Align();
+}
+
+// This struct is a compact representation of a valid (power of two) or
+// undefined (0) alignment.
+struct MaybeAlign : public llvm::Optional<Align> {
+private:
+  using UP = llvm::Optional<Align>;
+
+public:
+  // Do not perform checks in case of copy/move construct/assign, because the
+  // checks have been performed when building `Other`.
+  MaybeAlign(const MaybeAlign &Other) = default;
+  MaybeAlign &operator=(const MaybeAlign &Other) = default;
+  MaybeAlign(MaybeAlign &&Other) = default;
+  MaybeAlign &operator=(MaybeAlign &&Other) = default;
+
+  // Use llvm::Optional<Align> constructor.
+  using UP::UP;
+
+  explicit MaybeAlign(uint64_t Value) {
+    assert((Value == 0 || llvm::isPowerOf2_64(Value)) &&
+           "Alignment is not 0 or a power of 2");
+    if (Value)
+      emplace(Value);
+  }
+
+  // For convenience, returns a valid alignment or 1 if undefined.
+  Align valueOrOne() const { return hasValue() ? getValue() : Align(); }
+};
+
+// -----------------------------------------------------------------------------
+// isAligned: Checks that SizeInBytes is a multiple of the alignment.
+// -----------------------------------------------------------------------------
+
+inline bool isAligned(Align Lhs, uint64_t SizeInBytes) {
+  return SizeInBytes % Lhs.value() == 0;
+}
+
+// Returns false if the alignment is undefined.
+inline bool isAligned(MaybeAlign Lhs, uint64_t SizeInBytes) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return SizeInBytes % (*Lhs).value() == 0;
+}
+
+// -----------------------------------------------------------------------------
+// alignTo: Returns a multiple of A needed to store `Size` bytes.
+// -----------------------------------------------------------------------------
+
+inline uint64_t alignTo(uint64_t Size, Align A) {
+  return (Size + A.value() - 1) / A.value() * A.value();
+}
+
+// Returns `Size` if current alignment is undefined.
+inline uint64_t alignTo(uint64_t Size, MaybeAlign A) {
+  return A ? alignTo(Size, A.getValue()) : Size;
+}
+
+// -----------------------------------------------------------------------------
+// log2: Returns the log2 of the alignment.
+// e.g. Align(16).log2() == 4
+// -----------------------------------------------------------------------------
+
+inline unsigned Log2(Align A) { return A.ShiftValue; }
+
+/// \pre A must be defined.
+inline unsigned Log2(MaybeAlign A) {
+  ALIGN_CHECK_ISSET(A);
+  return Log2(A.getValue());
+}
+
+// -----------------------------------------------------------------------------
+// commonAlignment: returns the alignment that satisfies both alignments.
+// Same semantic as MinAlign.
+// -----------------------------------------------------------------------------
+
+inline Align commonAlignment(Align A, Align B) { return std::min(A, B); }
+
+inline Align commonAlignment(Align A, uint64_t Offset) {
+  return Align(MinAlign(A.value(), Offset));
+}
+
+inline MaybeAlign commonAlignment(MaybeAlign A, MaybeAlign B) {
+  return A && B ? commonAlignment(*A, *B) : A ? A : B;
+}
+
+inline MaybeAlign commonAlignment(MaybeAlign A, uint64_t Offset) {
+  return MaybeAlign(MinAlign((*A).value(), Offset));
+}
+
+// -----------------------------------------------------------------------------
+// Encode/Decode
+// -----------------------------------------------------------------------------
+
+// Returns a more compact representation of the alignment.
+// An undefined MaybeAlign is encoded as 0.
+inline unsigned encode(MaybeAlign A) { return A ? A->ShiftValue + 1 : 0; }
+
+// Dual operation of the encode function above.
+inline MaybeAlign decodeMaybeAlign(unsigned Value) {
+  if (Value == 0)
+    return MaybeAlign();
+  Align Out;
+  Out.ShiftValue = Value - 1;
+  return Out;
+}
+
+// Returns a more compact representation of the alignment.
+// The encoded value is positive by definition.
+// e.g. Align(1).encode() == 1
+// e.g. Align(16).encode() == 5
+inline unsigned encode(Align A) { return encode(MaybeAlign(A)); }
+
+// -----------------------------------------------------------------------------
+// Comparisons
+// -----------------------------------------------------------------------------
+
+// Comparisons between Align and scalars. Rhs must be positive.
+inline bool operator==(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() == Rhs;
+}
+inline bool operator!=(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() != Rhs;
+}
+inline bool operator<=(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() <= Rhs;
+}
+inline bool operator>=(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() >= Rhs;
+}
+inline bool operator<(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() < Rhs;
+}
+inline bool operator>(Align Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return Lhs.value() > Rhs;
+}
+
+// Comparisons between MaybeAlign and scalars.
+inline bool operator==(MaybeAlign Lhs, uint64_t Rhs) {
+  return Lhs ? (*Lhs).value() == Rhs : Rhs == 0;
+}
+inline bool operator!=(MaybeAlign Lhs, uint64_t Rhs) {
+  return Lhs ? (*Lhs).value() != Rhs : Rhs != 0;
+}
+inline bool operator<=(MaybeAlign Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return (*Lhs).value() <= Rhs;
+}
+inline bool operator>=(MaybeAlign Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return (*Lhs).value() >= Rhs;
+}
+inline bool operator<(MaybeAlign Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return (*Lhs).value() < Rhs;
+}
+inline bool operator>(MaybeAlign Lhs, uint64_t Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  ALIGN_CHECK_ISPOSITIVE(Rhs);
+  return (*Lhs).value() > Rhs;
+}
+
+// Comparisons operators between Align.
+inline bool operator==(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue == Rhs.ShiftValue;
+}
+inline bool operator!=(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue != Rhs.ShiftValue;
+}
+inline bool operator<=(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue <= Rhs.ShiftValue;
+}
+inline bool operator>=(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue >= Rhs.ShiftValue;
+}
+inline bool operator<(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue < Rhs.ShiftValue;
+}
+inline bool operator>(Align Lhs, Align Rhs) {
+  return Lhs.ShiftValue > Rhs.ShiftValue;
+}
+
+// Comparisons operators between Align and MaybeAlign.
+inline bool operator==(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() == (*Rhs).value();
+}
+inline bool operator!=(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() != (*Rhs).value();
+}
+inline bool operator<=(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() <= (*Rhs).value();
+}
+inline bool operator>=(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() >= (*Rhs).value();
+}
+inline bool operator<(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() < (*Rhs).value();
+}
+inline bool operator>(Align Lhs, MaybeAlign Rhs) {
+  ALIGN_CHECK_ISSET(Rhs);
+  return Lhs.value() > (*Rhs).value();
+}
+
+// Comparisons operators between MaybeAlign and Align.
+inline bool operator==(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() == Rhs.value();
+}
+inline bool operator!=(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() != Rhs.value();
+}
+inline bool operator<=(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() <= Rhs.value();
+}
+inline bool operator>=(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() >= Rhs.value();
+}
+inline bool operator<(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() < Rhs.value();
+}
+inline bool operator>(MaybeAlign Lhs, Align Rhs) {
+  ALIGN_CHECK_ISSET(Lhs);
+  return Lhs && (*Lhs).value() > Rhs.value();
+}
+
+// -----------------------------------------------------------------------------
+// Division
+// -----------------------------------------------------------------------------
+
+inline Align operator/(Align Lhs, uint64_t Divisor) {
+  assert(llvm::isPowerOf2_64(Divisor) &&
+         "Divisor must be positive and a power of 2");
+  assert(Lhs != 1 && "Can't halve byte alignment");
+  return Align(Lhs.value() / Divisor);
+}
+
+inline MaybeAlign operator/(MaybeAlign Lhs, uint64_t Divisor) {
+  assert(llvm::isPowerOf2_64(Divisor) &&
+         "Divisor must be positive and a power of 2");
+  return Lhs ? Lhs.getValue() / Divisor : MaybeAlign();
+}
+
+#undef ALIGN_CHECK_ISPOSITIVE
+#undef ALIGN_CHECK_ISSET
+
+} // namespace llvm
+
+#endif // LLVM_SUPPORT_ALIGNMENT_H_

llvm/trunk/unittests/Support/AlignmentTest.cpp

+//=== - llvm/unittest/Support/Alignment.cpp - Alignment utility tests -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Alignment.h"
+#include "gtest/gtest.h"
+
+#include <vector>
+
+using namespace llvm;
+
+namespace {
+
+std::vector<uint64_t> getValidAlignments() {
+  std::vector<uint64_t> Out;
+  for (size_t Shift = 0; Shift < 64; ++Shift)
+    Out.push_back(1ULL << Shift);
+  return Out;
+}
+
+// We use a subset of valid alignments for DEATH_TESTs as they are particularly
+// slow.
+std::vector<uint64_t> getValidAlignmentsForDeathTest() {
+  return {1, 1ULL << 31, 1ULL << 63};
+}
+
+std::vector<uint64_t> getNonPowerOfTwo() { return {3, 10, 15}; }
+
+TEST(Alignment, AlignDefaultCTor) { EXPECT_EQ(Align().value(), 1ULL); }
+
+TEST(Alignment, MaybeAlignDefaultCTor) {
+  EXPECT_FALSE(MaybeAlign().hasValue());
+}
+
+TEST(Alignment, ValidCTors) {
+  for (size_t Value : getValidAlignments()) {
+    EXPECT_EQ(Align(Value).value(), Value);
+    EXPECT_EQ((*MaybeAlign(Value)).value(), Value);
+  }
+}
+
+TEST(Alignment, InvalidCTors) {
+  EXPECT_DEATH((Align(0)), "Value must not be 0");
+  for (size_t Value : getNonPowerOfTwo()) {
+    EXPECT_DEATH((Align(Value)), "Alignment is not a power of 2");
+    EXPECT_DEATH((MaybeAlign(Value)), "Alignment is not 0 or a power of 2");
+  }
+}
+
+TEST(Alignment, CheckMaybeAlignHasValue) {
+  EXPECT_TRUE(MaybeAlign(1));
+  EXPECT_TRUE(MaybeAlign(1).hasValue());
+  EXPECT_FALSE(MaybeAlign(0));
+  EXPECT_FALSE(MaybeAlign(0).hasValue());
+  EXPECT_FALSE(MaybeAlign());
+  EXPECT_FALSE(MaybeAlign().hasValue());
+}
+
+TEST(Alignment, CantConvertUnsetMaybe) {
+  EXPECT_DEATH((MaybeAlign(0).getValue()), ".*");
+}
+
+TEST(Alignment, Division) {
+  for (size_t Value : getValidAlignments()) {
+    if (Value == 1) {
+      EXPECT_DEATH(Align(Value) / 2, "Can't halve byte alignment");
+      EXPECT_DEATH(MaybeAlign(Value) / 2, "Can't halve byte alignment");
+    } else {
+      EXPECT_EQ(Align(Value) / 2, Value / 2);
+      EXPECT_EQ(MaybeAlign(Value) / 2, Value / 2);
+    }
+  }
+  EXPECT_EQ(MaybeAlign(0) / 2, MaybeAlign(0));
+
+  EXPECT_DEATH(Align(8) / 0, "Divisor must be positive and a power of 2");
+  EXPECT_DEATH(Align(8) / 3, "Divisor must be positive and a power of 2");
+}
+
+TEST(Alignment, AlignTo) {
+  struct {
+    uint64_t alignment;
+    uint64_t offset;
+    uint64_t rounded;
+  } kTests[] = {
+      // MaybeAlign
+      {0, 0, 0},
+      {0, 1, 1},
+      {0, 5, 5},
+      // MaybeAlign / Align
+      {1, 0, 0},
+      {1, 1, 1},
+      {1, 5, 5},
+      {2, 0, 0},
+      {2, 1, 2},
+      {2, 2, 2},
+      {2, 7, 8},
+      {2, 16, 16},
+      {4, 0, 0},
+      {4, 1, 4},
+      {4, 4, 4},
+      {4, 6, 8},
+  };
+  for (const auto &T : kTests) {
+    MaybeAlign A(T.alignment);
+    // Test MaybeAlign
+    EXPECT_EQ(alignTo(T.offset, A), T.rounded);
+    // Test Align
+    if (A)
+      EXPECT_EQ(alignTo(T.offset, A.getValue()), T.rounded);
+  }
+}
+
+TEST(Alignment, Log2) {
+  for (size_t Value : getValidAlignments()) {
+    EXPECT_EQ(Log2(Align(Value)), Log2_64(Value));
+    EXPECT_EQ(Log2(MaybeAlign(Value)), Log2_64(Value));
+  }
+  EXPECT_DEATH(Log2(MaybeAlign(0)), ".* should be defined");
+}
+
+TEST(Alignment, MinAlign) {
+  struct {
+    uint64_t A;
+    uint64_t B;
+    uint64_t MinAlign;
+  } kTests[] = {
+      // MaybeAlign
+      {0, 0, 0},
+      {0, 8, 8},
+      {2, 0, 2},
+      // MaybeAlign / Align
+      {1, 2, 1},
+      {8, 4, 4},
+  };
+  for (const auto &T : kTests) {
+    EXPECT_EQ(commonAlignment(MaybeAlign(T.A), MaybeAlign(T.B)), T.MinAlign);
+    EXPECT_EQ(MinAlign(T.A, T.B), T.MinAlign);
+    if (T.A)
+      EXPECT_EQ(commonAlignment(Align(T.A), MaybeAlign(T.B)), T.MinAlign);
+    if (T.B)
+      EXPECT_EQ(commonAlignment(MaybeAlign(T.A), Align(T.B)), T.MinAlign);
+    if (T.A && T.B)
+      EXPECT_EQ(commonAlignment(Align(T.A), Align(T.B)), T.MinAlign);
+  }
+}
+
+TEST(Alignment, Encode_Decode) {
+  for (size_t Value : getValidAlignments()) {
+    {
+      Align Actual(Value);
+      Align Expected = decodeMaybeAlign(encode(Actual)).getValue();
+      EXPECT_EQ(Expected, Actual);
+    }
+    {
+      MaybeAlign Actual(Value);
+      MaybeAlign Expected = decodeMaybeAlign(encode(Actual));
+      EXPECT_EQ(Expected, Actual);
+    }
+  }
+  MaybeAlign Actual(0);
+  MaybeAlign Expected = decodeMaybeAlign(encode(Actual));
+  EXPECT_EQ(Expected, Actual);
+}
+
+TEST(Alignment, isAligned) {
+  struct {
+    uint64_t alignment;
+    uint64_t offset;
+    bool isAligned;
+  } kTests[] = {
+      // MaybeAlign / Align
+      {1, 0, true},  {1, 1, true},  {1, 5, true},  {2, 0, true},
+      {2, 1, false}, {2, 2, true},  {2, 7, false}, {2, 16, true},
+      {4, 0, true},  {4, 1, false}, {4, 4, true},  {4, 6, false},
+  };
+  for (const auto &T : kTests) {
+    MaybeAlign A(T.alignment);
+    // Test MaybeAlign
+    EXPECT_EQ(isAligned(A, T.offset), T.isAligned);
+    // Test Align
+    if (A)
+      EXPECT_EQ(isAligned(A.getValue(), T.offset), T.isAligned);
+  }
+}
+
+TEST(Alignment, AlignComparisons) {
+  std::vector<size_t> ValidAlignments = getValidAlignments();
+  std::sort(ValidAlignments.begin(), ValidAlignments.end());
+  for (size_t I = 1; I < ValidAlignments.size(); ++I) {
+    assert(I >= 1);
+    const Align A(ValidAlignments[I - 1]);
+    const Align B(ValidAlignments[I]);
+    EXPECT_EQ(A, A);
+    EXPECT_NE(A, B);
+    EXPECT_LT(A, B);
+    EXPECT_GT(B, A);
+    EXPECT_LE(A, B);
+    EXPECT_GE(B, A);
+    EXPECT_LE(A, A);
+    EXPECT_GE(A, A);
+
+    EXPECT_EQ(A, A.value());
+    EXPECT_NE(A, B.value());
+    EXPECT_LT(A, B.value());
+    EXPECT_GT(B, A.value());
+    EXPECT_LE(A, B.value());
+    EXPECT_GE(B, A.value());
+    EXPECT_LE(A, A.value());
+    EXPECT_GE(A, A.value());
+
+    EXPECT_EQ(std::max(A, B), B);
+    EXPECT_EQ(std::min(A, B), A);
+
+    const MaybeAlign MA(ValidAlignments[I - 1]);
+    const MaybeAlign MB(ValidAlignments[I]);
+    EXPECT_EQ(MA, MA);
+    EXPECT_NE(MA, MB);
+    EXPECT_LT(MA, MB);
+    EXPECT_GT(MB, MA);
+    EXPECT_LE(MA, MB);
+    EXPECT_GE(MB, MA);
+    EXPECT_LE(MA, MA);
+    EXPECT_GE(MA, MA);
+
+    EXPECT_EQ(MA, MA ? (*MA).value() : 0);
+    EXPECT_NE(MA, MB ? (*MB).value() : 0);
+    EXPECT_LT(MA, MB ? (*MB).value() : 0);
+    EXPECT_GT(MB, MA ? (*MA).value() : 0);
+    EXPECT_LE(MA, MB ? (*MB).value() : 0);
+    EXPECT_GE(MB, MA ? (*MA).value() : 0);
+    EXPECT_LE(MA, MA ? (*MA).value() : 0);
+    EXPECT_GE(MA, MA ? (*MA).value() : 0);
+
+    EXPECT_EQ(std::max(A, B), B);
+    EXPECT_EQ(std::min(A, B), A);
+  }
+}
+
+TEST(Alignment, AssumeAligned) {
+  EXPECT_EQ(assumeAligned(0), Align(1));
+  EXPECT_EQ(assumeAligned(0), Align());
+  EXPECT_EQ(assumeAligned(1), Align(1));
+  EXPECT_EQ(assumeAligned(1), Align());
+}
+
+TEST(Alignment, ComparisonsWithZero) {
+  for (size_t Value : getValidAlignmentsForDeathTest()) {
+    EXPECT_DEATH((void)(Align(Value) == 0), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) != 0), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) >= 0), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) <= 0), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) > 0), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) < 0), ".* should be defined");
+  }
+}
+
+TEST(Alignment, CompareMaybeAlignToZero) {
+  for (size_t Value : getValidAlignmentsForDeathTest()) {
+    // MaybeAlign is allowed to be == or != 0
+    (void)(MaybeAlign(Value) == 0);
+    (void)(MaybeAlign(Value) != 0);
+    EXPECT_DEATH((void)(MaybeAlign(Value) >= 0), ".* should be defined");
+    EXPECT_DEATH((void)(MaybeAlign(Value) <= 0), ".* should be defined");
+    EXPECT_DEATH((void)(MaybeAlign(Value) > 0), ".* should be defined");
+    EXPECT_DEATH((void)(MaybeAlign(Value) < 0), ".* should be defined");
+  }
+}
+
+TEST(Alignment, CompareAlignToUndefMaybeAlign) {
+  for (size_t Value : getValidAlignmentsForDeathTest()) {
+    EXPECT_DEATH((void)(Align(Value) == MaybeAlign(0)), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) != MaybeAlign(0)), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) >= MaybeAlign(0)), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) <= MaybeAlign(0)), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) > MaybeAlign(0)), ".* should be defined");
+    EXPECT_DEATH((void)(Align(Value) < MaybeAlign(0)), ".* should be defined");
+  }
+}
+
+} // end anonymous namespace

llvm/trunk/unittests/Support/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_llvm_unittest(SupportTests
+  AlignmentTest.cpp
   AlignOfTest.cpp
   AllocatorTest.cpp
   AnnotationsTest.cpp
   ARMAttributeParser.cpp
   ArrayRecyclerTest.cpp
   BinaryStreamTest.cpp
   BlockFrequencyTest.cpp
   BranchProbabilityTest.cpp