Page MenuHomePhabricator

[Attributor] Derive better alignment for accessed pointers
ClosedPublic

Authored by jdoerfert on Mar 24 2020, 12:39 AM.

Details

Summary

Use DL & ABI information for better alignment deduction, e.g., if a type
is accessed and the ABI specifies an alignment requirement for such an
access we can use it. This is based on a patch by @lebedev.ri and
inspired by getBaseAlign in Loads.cpp.

Depends on D76673.

Diff Detail

Event Timeline

jdoerfert created this revision.Mar 24 2020, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2020, 12:39 AM

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

Hm, so we can only look at the abi if the load alignment is unspecified?

jdoerfert updated this revision to Diff 252239.Mar 24 2020, 1:01 AM

Repsect underaligned accesses, already tested in Transforms/Attributor/ArgumentPromotion/alignment.ll

jdoerfert updated this revision to Diff 252240.Mar 24 2020, 1:03 AM

Simplify helper code

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

Hm, so we can only look at the abi if the load alignment is unspecified?

I strongly believe so, otherwise there would be no point in specifying an alignment
smaller than the abi alignment on load/store, and no way to do underaligned loads/stores,
which is obviously not the case.

lebedev.ri accepted this revision.Mar 24 2020, 1:14 AM

This looks good to me, thanks for relieving me from having to upstream this myself :)

llvm/lib/Transforms/IPO/Attributor.cpp
3900–3914

information

This revision is now accepted and ready to land.Mar 24 2020, 1:14 AM

That's the patch i showed, yes :)
I'm not sure this is sound.
Given underaligned load, we can't just take max(align of load, abi alignment)

Hm, so we can only look at the abi if the load alignment is unspecified?

I strongly believe so, otherwise there would be no point in specifying an alignment
smaller than the abi alignment on load/store, and no way to do underaligned loads/stores,
which is obviously not the case.

Agreed. We even had a test case(Transforms/Attributor/ArgumentPromotion/alignment.ll). Fixed the code.

Harbormaster completed remote builds in B50221: Diff 252240.
Harbormaster completed remote builds in B50220: Diff 252239.
uenoku added a comment.EditedMar 26 2020, 9:50 AM

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

lebedev.ri added a comment.EditedMar 26 2020, 10:33 AM

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

See my previous comment.
If we decide that pointer has ABI alignment from the get go,
then we will immediately update every underaligned load/store to no longer be underaligned,
which makes no sense. Unless i'm horribly mistaken, just because we said that %z is i32* or i512*,
it doesn't mean it is UB for it to be aligned to a single byte (& 0b1 == 1),
it is the access to such misaligned pointer that is UB.

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

See my previous comment.
If we decide that pointer has ABI alignment from the get go,
then we will immediately update every underaligned load/store to no longer be underaligned,
which makes no sense. Unless i'm horribly mistaken, just because we said that %z is i32* or i512*,
it doesn't mean it is UB for it to be aligned to a single byte (& 0b1 == 1),
it is the access to such misaligned pointer that is UB.

I got it. This makes sense. Thank you.

This revision was automatically updated to reflect the committed changes.