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

[Eduard Burtescu via llvm-commits]

eddyb added a comment.

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.


================
Comment at: lib/Analysis/ValueTracking.cpp:3260
@@ +3259,3 @@
+    unsigned BaseAlign = DL.getABITypeAlignment(BaseType);
+    if (BaseAlign < Align)
+      BaseAlign = Align;
----------------
eddyb wrote:
> dblaikie wrote:
> > Where did this case come from?
> 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.
> **@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.

================
Comment at: lib/Analysis/ValueTracking.cpp:3308
@@ -3307,3 +3302,1 @@
 
-  if (Ty->isSized()) {
-    APInt Offset(DL.getTypeStoreSizeInBits(VTy), 0);
----------------
eddyb wrote:
> dblaikie wrote:
> > Where'd this check go?
> 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.
> **@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.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1779
@@ -1778,3 +1778,3 @@
     // isDereferenceablePointer -> deref attribute
-    if (isDereferenceablePointer(DerivedPtr, DL)) {
+    if (isDereferenceablePointer(DerivedPtr, 1, DL)) {
       if (Argument *A = dyn_cast<Argument>(DerivedPtr)) {
----------------
eddyb wrote:
> dblaikie wrote:
> > Why is it only '1', rather than 'Bytes' used below?
> I recall this being `i8*`, but if that is not the case, I'll use `Bytes` instead.
> **@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).


http://reviews.llvm.org/D16425