This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Constant pools need 4-byte alignment if we only have tADR
ClosedPublic

Authored by john.brawn on Sep 2 2022, 5:35 AM.

Details

Summary

When the only ADR instruction we have is the 16-bit thumb one then all constant pool entries need to be 4-byte aligned, as tADR has an offset that's a multiple of 4.

It looks like previously there happened to be no situations in which we encountered a constant pool entry with alignment less than 4, so failing to do this didn't cause any problems, but the expansion of cttz to a table does use a constant pool with alignment 1, so we now need to handle it correctly.

Diff Detail

Event Timeline

john.brawn created this revision.Sep 2 2022, 5:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 5:35 AM
john.brawn requested review of this revision.Sep 2 2022, 5:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2022, 5:35 AM

the expansion of cttz to a table

Can you explain what that means?

How does the test demostrate the difference in alignment compared to the old codegen? Is it the .p2align 2 directive?

the expansion of cttz to a table

Can you explain what that means?

D128911 made cttz be expanded to a table lookup, with the table being placed in a constant pool.

How does the test demostrate the difference in alignment compared to the old codegen? Is it the .p2align 2 directive?

Yes, it's the .p2align 2. Without that if you try to generate an object (https://godbolt.org/z/hdGsPccn8) you get "error: misaligned pc-relative fixup value".

labrinea accepted this revision.Sep 5 2022, 4:47 PM

Ok, I looked at the other patch; it'd be helpful to mention it on the description. This change seems fine. Thanks.

This revision is now accepted and ready to land.Sep 5 2022, 4:47 PM
dmgreen accepted this revision.Sep 6 2022, 12:06 AM
dmgreen added a subscriber: dmgreen.

I was expecting this to be in ConstantIslandPass, but this sounds OK. LGTM, thanks for the fix.

This revision was landed with ongoing or failed builds.Sep 6 2022, 3:36 AM
This revision was automatically updated to reflect the committed changes.