Page MenuHomePhabricator

[opaque pointer types] [NFC] isDereferenceable{,AndAligned}Pointer: take the accessed size (and alignment) as arguments.
Needs ReviewPublic

Authored by eddyb on Jan 21 2016, 12:08 PM.

Details

Reviewers
dblaikie
mjacob

Diff Detail

Event Timeline

eddyb updated this revision to Diff 45580.Jan 21 2016, 12:08 PM
eddyb retitled this revision from to [opaque pointer types] [NFC] isDereferenceable{,AndAligned}Pointer: take the accessed size (and alignment) as arguments..
eddyb updated this object.
eddyb added reviewers: mjacob, dblaikie.
eddyb added a subscriber: llvm-commits.
mjacob added inline comments.Jan 21 2016, 5:42 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
432

Can we pass the size instead of pointee type here?

eddyb added inline comments.Jan 21 2016, 5:47 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
432

Yes, but the method is local to this file and all the callers already have a type.
Passing a size means using DL.getTypeStoreSize at each call site, which is anti-DRY IMO.

dblaikie added inline comments.Jan 21 2016, 6:03 PM
include/llvm/Analysis/ValueTracking.h
244

Do we still need to pass the DataLyaout to these functions if they have the size & alignment?

lib/Analysis/ValueTracking.cpp
3260

Where did this case come from?

3308

Where'd this check go?

lib/Transforms/InstCombine/InstCombineCalls.cpp
1779

Why is it only '1', rather than 'Bytes' used below?

eddyb added inline comments.Jan 21 2016, 6:14 PM
include/llvm/Analysis/ValueTracking.h
244

Yes, it's used all over the place in ValueTracking (to get the sizes of other types found during the process).

lib/Analysis/ValueTracking.cpp
3260

Previously, if Align was 0, then BaseAlign would take over.
Instead, I'm taking the maximum alignment required by either, but I'm not 100% sure it does the exact same thing, only that it passes the tests.

3308

Based on the fact that you can't get the Size necessary to call this function if you have an unsized type.

But I can add the check back, as if (Size > 0), if you want.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1779

I recall this being i8*, but if that is not the case, I'll use Bytes instead.

mjacob added inline comments.Jan 21 2016, 6:24 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
432

This is a tradeoff we have to make anyway. E.g. in this revision you introduce more calls to DL.getTypeStoreSize() than you remove. IMHO there is no way to avoid this if we want to make pointers opaque.

Anyway, isSafeToPromoteArgument() has only two callers, one of these is the method itself.

eddyb added inline comments.Jan 21 2016, 6:39 PM
lib/Transforms/IPO/ArgumentPromotion.cpp
432

Odd, I must've misread something else, I thought I saw half a dozen calls. Fair enough then.

eddyb added a comment.Jan 21 2016, 9:15 PM

Sorry @dblaikie, but I almost missed these comments as they weren't showing up in Differential.

I've tried to manually quote them here so that the review makes sense.

lib/Analysis/ValueTracking.cpp
3260

@dblaikie (via email)
I think it'd be best to preserve the old behavior. Feel free to add a
// FIXME: Is this really the right thing, shouldn't we use the blah blah blah
or the like.

There are only 4 possible cases, for a GEP getting a T field in S:

  • No explicit align: before was alignof(S)
    • alignof(S) >= alignof(T): now alignof(S) (same)
    • alignof(S) < alignof(T): now alignof(T) (larger, i.e. overly conservative)
  • Explicit align (A): before was A
    • alignof(S) <= A: now A (same)
    • alignof(S) > A: now alignof(S) (larger, i.e. overly conservative)

The alignof(S) < alignof(T) case seems interesting to me: can an aggregate have a smaller alignment than one of its fields?

And I'm not sure what, if any, bounds are placed on explicit alignments.

So let's say I want to preserve the old behavior.
I could pass *both* the ABI alignment and the explicit alignment, but that would be even noisier.
Or I could bundle the alignment value and a boolean flag indicating whether it's ABI or explicit.

But we need to discuss which semantic is *correct*:

If Base is aligned to Align and Offset is a multiple of Align (checked below), then the resulting pointer should still be aligned.

For this to have been safe in the old implementation, alignof(S) >= alignof(T) must hold, otherwise an aligned Base might not result in an aligned field.

However, if alignof(S) > alignof(T), and no explicit alignment was provided, the old implementation was overly conservative itself and required that Base be aligned to alignof(S) even though alignof(T) would suffice.

3308

@dblaikie (via email)
So was this check unnecessary all along? Or are callers doing an equivalent check now?

All callers I've encountered would now trip an assert were this case to occur, so at least it wasn't tested against.
It might be interesting to see which callers could actually end up with a non-sized type.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1779

@dblaikie (via email)
If it's obviously i8* in the code, that's OK (could you tell me where to look - even if it's a few lines nearby, that'd be helpful), might be worth a comment or assertion.

Ah, I remember now, the check here is actually that if isDereferenceablePointer returns true, and we have an Argument, the information must come from a dereferenceable attribute.

I suppose it might be better to just skip isDereferenceablePointer and just copy the dereferenceable attribute.

This intrinsic (experimental.gc.relocate) is one of the ones which happen to keep all of their pointee information behind a pointer type and will need to be redesigned (slightly).