indexXArrayAsY# primops make no attempt at handling alignment

It appears that the unaligned index and store primops added in #14447 (closed) (see efd70cfb) make no attempt at dealing with architectures that do not permit unaligned memory access. This is currently only problematic when using the LLVM backend to target ARMv7.

added ARM Phighest Tbug runtime crash labels

assigned to @bgamari

marked this issue as related to #14447 (closed)

marked this issue as related to #20987 (closed)

My approach for fixing this will be to introduce two new MachOps:

• MO_UnalignedLoad :: CmmType -> MachOp
• MO_UnalignedStore :: CmmType -> CallishMachOp

These will be lowered to normal aligned operations in our current set of NCGs. In the LLVM backend we will annotate the produced load or store instruction with an align declaration.

Happily, this could also allow us to fix #1016.

Unhappily, this may interfere with some optimisations (namely sinking)

There will be lowered to normal aligned operations in our current set of NCGs.

Both of them? The sentence is a bit unclear on that point. I would assume making knowledge about alignment of part of CmmLoad/Store would be a bit easier implementation wise rather than making it it's own primop. But maybe I'm wrong on that.

Unhappily, this may interfere with some optimisations (namely sinking)

Could you expand on that? It's not obvious at a glance why they should negative affect sinking.

I would assume making knowledge about alignment of part of CmmLoad/Store would be a bit easier implementation wise

It's not so clear that this is true, actually. In particular, the naturally-aligned case is overwhelmingly more common. Having to deal with the potentially-unaligned case in all uses of CmmLoad and CmmStore seemed unnecessarily burdensome. That being said, on further reflection I'm beginning to come around to it being the more principled option

marked this issue as related to #1016

changed the description

Another potential problem-spot here is data constructor packing, since we will now (as of cca2d6b7) allow packing of multiple non-pointer sub-word-size fields into a single constructor field word. I do not currently know if the implementation currently makes any attempt at addressing alignment.

Thankfully the data constructor packing logic correctly handles alignment (see cca2d6b7)

mentioned in merge request !7437 (closed)

This morning I discussed the above approach with @AndreasK, who argued that it be preferrable to rather extend CmmStore and CmmLoad with alignment information. While originally I rejected this approach as being a very invasive change considering the corner-case we are addressing, @AndreasK argued convincingly that the correctness benefits to handling unaligned and aligned accesses consistently we would worth the cost.

I have implemented this in !7440 (closed). Unfortunately, I quickly found when attempting to test the patch on real hardware that GHC 8.10.7 cannot build Hadrian on ARMv7 due to misalignment. I've reverted to using make to test the patch, but this further increases the urgency of fixing this issue and getting it into users' hands.

mentioned in merge request !7440 (closed)

This should be fixed with !7440 (closed).

closed

mentioned in issue #11143 (closed)

mentioned in issue #21295 (closed)