GEP transformation by InstCombiner

Hi all,

I’m working on an out-of-tree target and encountered the following problem:

InstCombiner “normalizes” GEPs and extends Index operand to the Pointer width.
It works fine if you can convert pointer to integer for address calculation and I assume that all registered targets do this.

The target I’m working on has very restricted ISA for the pointer calculation:
ptr + int, ptr - int, ptr - ptr and ptr-compare

I have full arithmetic set for 32-bit integers, but the Ptr is wider. Extending index to the Ptr width requires full arithmetic support for pointers.
But, actually, it does not come from C-sources (casting Ptr to int means truncation).

I’d like to add TTI (TargetTransformInfo) to InstCombiner in order to configure the width of GEP indices.
The current default behavior will be preserved.
What do you think?

Thanks.

  • Elena

Given that this affects the canonical form of the IR, based on our current practice, it should go in DataLayout (not in TTI). InstCombine should probably know how to do the right thing for the IR even if the particular target is not compiled in. -Hal

I tried to retrieve anything from DataLayout. It contains pointer size, but how can I conclude that the GEP index can’t be widened?

I meant that we’d add a new field giving the preferred size for indexing arithmetic. On the other hand, in your case, and in general, would it make sense to prevent widening beyond the largest legal integer type? The legal integer types are already in DataLayout. -Hal

On the other hand, in your case, and in general, would it make sense to prevent widening beyond the largest legal integer type? The legal integer types are already in DataLayout.

Yes, it sounds right if it does not contradict other targets behavior. I’ll check.

Thank you.

Hi all,

I’m working on an out-of-tree target and encountered the following problem:

InstCombiner “normalizes” GEPs and extends Index operand to the Pointer width.
It works fine if you can convert pointer to integer for address calculation and I assume that all registered targets do this.

The target I’m working on has very restricted ISA for the pointer calculation:
ptr + int, ptr - int, ptr - ptr and ptr-compare

This sounds very familiar - that’s exactly the set of operations that CHERI supports.

We have a set of out-of-tree patches that extend the data layout to understand that there’s a difference between the size and the range of a pointer (e.g. a 128-bit pointer can store 64-bit addresses + metadata) and fixes for all of the optimisers that we’ve found, and SelectionDAG. We add explicit PTRADD, INTTOPTR and PTRTOINT nodes in SelectionDAG for architectures where pointer+integer is distinct from integer arithmetic and where inttoptr / ptrtoint are not bitcasts.

I have full arithmetic set for 32-bit integers, but the Ptr is wider. Extending index to the Ptr width requires full arithmetic support for pointers.
But, actually, it does not come from C-sources (casting Ptr to int means truncation).

We also have clang patches that ensure that these operations do something meaningful.

I’d like to add TTI (TargetTransformInfo) to InstCombiner in order to configure the width of GEP indices.
The current default behavior will be preserved.
What do you think?

We solved this by adding an f qualifier to the p attribute in our DataLayout to describe pointers that are not integers and use address space 0 to define the range. This isn’t quite ideal, and we should probably explicitly add the range to pointers that are wider than integers.

Note that InstCombine is not the only place that tries to insert pointer-width GEPs in the optimisation pipeline. I think that we’ve fixed all of them, but I can’t be entirely sure.

We haven’t upstreamed this, because no in-tree target needs them, but I’d be happy to try to extract the relevant parts and put them up for review if this is generally useful.

David

Note that InstCombine is not the only place that tries to insert pointer-width GEPs in the optimization pipeline. I think that we’ve fixed all of them, but I can’t be entirely sure.

I'm going to upload a patch, but I'm fixing only the places that are covered by our test system. I'll add you as a reviewer and you are welcome to help me with fixing them all.

We haven’t upstreamed this, because no in-tree target needs them, but I’d be happy to try to extract the relevant parts and put them up for review if this is generally useful.

We also have an internal diff, but we are trying to upstream everything that has a common sense.

- Elena

Please take a look at our tree:

https://github.com/CTSRD-CHERI/llvm/

We’ve been doing this in public for the last five to six years and are able to compile large bodies of C/C++ code with it. It would be a shame to reinvent the wheel.

I’m more than happy for anything that’s generally useful to go upstream.

David