This is an archive of the discontinued LLVM Phabricator instance.

Analysis: Fix assertion when load alignment exceeds address space size
ClosedPublic

Authored by arsenm on Jun 19 2023, 3:57 PM.

Details

Summary

Apparently the maximum alignment no longer fits in 32-bits now, which
overflows a 32-bit offset and would fail on the isPowerOf2 assert.

Diff Detail

Event Timeline

arsenm created this revision.Jun 19 2023, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 3:57 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
arsenm requested review of this revision.Jun 19 2023, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2023, 3:57 PM
Herald added a subscriber: wdng. · View Herald Transcript
gchatelet added inline comments.Jun 20 2023, 4:52 AM
llvm/lib/Analysis/Loads.cpp
33

How about the following code:

static bool isAligned(const Value *Base, const APInt &Offset, Align Alignment,
                      const DataLayout &DL) {
  Align BA = Base->getPointerAlignment(DL);
  if (BA < Alignment)
    return false;
  const unsigned OffsetTrailingZeroes = Offset.countr_zero();
  const unsigned MinimumTrailingZeroes = Log2(Alignment);
  return OffsetTrailingZeroes >= MinimumTrailingZeroes;
}

FYI Log2(Alignment) is free.

arsenm updated this revision to Diff 532875.Jun 20 2023, 6:02 AM
arsenm marked an inline comment as done.

Use suggestion

Actually, it should be return OffsetTrailingZeroes >= MinimumTrailingZeroes || Offset.isZero(); because zero is always aligned to whatever power of two.
It probably makes sense to add a helper function in Alignment.h. I'll take care of it.

arsenm updated this revision to Diff 535778.Jun 29 2023, 7:04 AM

Use isAligned

gchatelet added inline comments.Jun 29 2023, 7:31 AM
llvm/lib/Analysis/Loads.cpp
34–35

Now that the code is in APInt it's not entirely obvious how the comment is addressed.
Maybe reformulating the comment like so would help : "Delegating alignment check to APInt as Offset and Alignment might have different bit width. e.g., Align is 2^64 but Offset is from a 32 bit Address Space."
Or something along those lines.

arsenm updated this revision to Diff 535932.Jun 29 2023, 11:28 AM

Drop comment

gchatelet accepted this revision.Jun 30 2023, 12:51 AM
This revision is now accepted and ready to land.Jun 30 2023, 12:51 AM