This is an archive of the discontinued LLVM Phabricator instance.

The maximal representable alignment in LLVM IR is 1GiB, not 512MiB
ClosedPublic

Authored by lebedev.ri on Aug 24 2021, 1:35 PM.

Details

Summary

In IR bitcode, the alignment is stored log2, biased by one (to represent 'default' alignment),
in 5 bits (would one/two extra bits cause the sky to fall?).
But that means that the maximal alignment exponent is (1<<5)-2, which is 30, not 29.
And indeed, alignment of 1073741824 rountrips IR serialization-deserialization.

While this doesn't seem all that important, this doubles the maximal supported alignment
from 512MiB to 1GiB, and there's actually one noticeable use-case for that;
On X86, the huge pages can have sizes of 2MiB and 1GiB (!).

So while this doesn't add support for truly huge alignments,
i think this adds zero-cost support for a not-trivially-dismissable case.

I don't believe we need any upgrade infrastructure,
and since we don't explicitly record the IR version, we don't need to bump one.

Diff Detail

Event Timeline

lebedev.ri created this revision.Aug 24 2021, 1:35 PM
lebedev.ri requested review of this revision.Aug 24 2021, 1:35 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
lebedev.ri edited the summary of this revision. (Show Details)Aug 24 2021, 1:53 PM

Wild speculation. This may be a historical artifact of LoadInst and StoreInst having their getAlignment() function written like this when this limit was created

unsigned getAlignment() const {
  return (1 << (getSubclassDataFromInstruction() >> 1)) >> 1;
}

Because "1" is signed int the left shift produced a signed int. The final right shift would be an arithmetic shift. So if the alignment was 1 << 30, the subclass data would be 31, the shift would produce INT_MIN, and the arithmetic right shift would produce 0xc0000000 which would be incorrect.

AllocaInst used 1U instead of 1 so would have been immune.

Could use some MIR tests to make sure that parser doesn't explode

Could use some MIR tests to make sure that parser doesn't explode

Added.

arsenm added inline comments.Aug 25 2021, 1:18 PM
llvm/test/CodeGen/X86/load-with-1gb-alignment.mir
1

Should use -run-pass=none, and go in test/CodeGen/MIR/X86

6–16

Don't need the IR section

31

Don't need the IR reference, can also test basealign

@arsenm i do not understand how i can avoid having IR there, but addressed other nits. thanks for taking a look!

Address rest of nits.

lebedev.ri marked 3 inline comments as done.Aug 25 2021, 2:15 PM

Seems reasonable to me.

@arsenm are you happy with the test coverage here?

Wild speculation. This may be a historical artifact of LoadInst and StoreInst having their getAlignment() function written like this when this limit was created

unsigned getAlignment() const {
  return (1 << (getSubclassDataFromInstruction() >> 1)) >> 1;
}

Because "1" is signed int the left shift produced a signed int. The final right shift would be an arithmetic shift. So if the alignment was 1 << 30, the subclass data would be 31, the shift would produce INT_MIN, and the arithmetic right shift would produce 0xc0000000 which would be incorrect.

AllocaInst used 1U instead of 1 so would have been immune.

That does seem plausible :)

arsenm added inline comments.Aug 25 2021, 2:44 PM
llvm/test/CodeGen/MIR/X86/load-with-1gb-alignment.mir
1 ↗(On Diff #368722)

Needs to add -march/-mtriple

lebedev.ri marked an inline comment as done.

Add -march=x86-64 that i forgot :/

Thanks everyone!
I don't expect that there will be any further feedback, so i plan on landing this tomorrow, unless there is further feedback before then.

llvm/test/CodeGen/MIR/X86/load-with-1gb-alignment.mir
1 ↗(On Diff #368722)

D'oh.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 26 2021, 2:54 AM
This revision was automatically updated to reflect the committed changes.