This is an archive of the discontinued LLVM Phabricator instance.

[IR] Add helper to convert offset to GEP indices
ClosedPublic

Authored by nikic on Sep 19 2021, 1:01 PM.

Details

Summary

We implement logic to convert a byte offset into a sequence of GEP indices for that offset in a number of places. This patch adds a DataLayout::getGEPIndicesForOffset() method, which implements the core logic. I've updated SROA, ConstantFolding and InstCombine to use it, and there's a few more places where it looks relevant.

Diff Detail

Event Timeline

nikic created this revision.Sep 19 2021, 1:01 PM
nikic requested review of this revision.Sep 19 2021, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2021, 1:01 PM
aeubanks added inline comments.Sep 19 2021, 10:56 PM
llvm/include/llvm/IR/DataLayout.h
583

residual

584

getGEPIndicesForOffset

llvm/lib/Analysis/ConstantFolding.cpp
1008

what do you mean by this?

1021

clang-format?

llvm/lib/IR/DataLayout.cpp
913

assert(Offset.isPositive())?
or can this be negative?

nikic updated this revision to Diff 373632.Sep 20 2021, 9:58 AM
nikic marked 4 inline comments as done.
nikic edited the summary of this revision. (Show Details)

Address comments.

nikic added inline comments.Sep 20 2021, 9:59 AM
llvm/lib/Analysis/ConstantFolding.cpp
1008

We add extra zero indices in the hope that can return ResType without inserting a bitcast. But if that's not actually possible, we'll just add the maximum number of zeros and insert the bitcast anyway.

aeubanks accepted this revision.Sep 20 2021, 10:33 AM
aeubanks added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
1008

ah I didn't see the bitcast below

This revision is now accepted and ready to land.Sep 20 2021, 10:33 AM
This revision was landed with ongoing or failed builds.Sep 20 2021, 11:20 AM
This revision was automatically updated to reflect the committed changes.

Hi @nikic,

This (admittedly silly) example starts hitting an assertion with this patch:

opt -passes='globalopt' -S -o - very_large_offset.ll

It fails with:

opt: ../lib/IR/DataLayout.cpp:913: void addElementIndex(SmallVectorImpl<llvm::APInt> &, llvm::TypeSize, llvm::APInt &): Assertion `Offset.isNonNegative() && "Remaining offset shouldn't be negative"' failed.

The input contains a huge (larger than half the address space) array @s and also a Gep with a huge offset into that object so possibly this is all UB but I suppose in a very nice world the compiler shouldn't crash with an assertion anyway since I guess the input is legal?

Hi @nikic,

This (admittedly silly) example starts hitting an assertion with this patch:

opt -passes='globalopt' -S -o - very_large_offset.ll

It fails with:

opt: ../lib/IR/DataLayout.cpp:913: void addElementIndex(SmallVectorImpl<llvm::APInt> &, llvm::TypeSize, llvm::APInt &): Assertion `Offset.isNonNegative() && "Remaining offset shouldn't be negative"' failed.

The input contains a huge (larger than half the address space) array @s and also a Gep with a huge offset into that object so possibly this is all UB but I suppose in a very nice world the compiler shouldn't crash with an assertion anyway since I guess the input is legal?

Seems like ElemSize is greater than 2^32 and we're getting some weird math in addElementIndex().