[PATCH] D80072: Make Value::getPointerAlignment() return an Align, not a MaybeAlign.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added subscribers: fhahn, spatel, reames, nlopes, arsenm, lebedev.ri.
jdoerfert added inline comments.


================
Comment at: llvm/lib/Analysis/Loads.cpp:40
+      BA = DL.getABITypeAlign(Ty);
   }
+  const APInt APAlign(Offset.getBitWidth(), Alignment.value());
----------------
efriedma wrote:
> jdoerfert wrote:
> > As you noted, this seems wrong. I think this allows wrong results for `llvm::isDereferenceableAndAlignedPointer` queries. What happens if you remove this boosting? 
> > 
> > --- 
> > 
> > From D9791: 
> > > I assume that pointers without explicit alignment have ABI alignment.
> > 
> > I think this is OK for pointers accessed by loads without specified alignment but not for "pointers" in the general sense. If you look at the AttributorAttributes.cpp change, the function there does something similar to the `getBaseAlign` on the left but it only does so for pointers accessed without specified alignment. (= I think I came to this conclusion for the second time.)
> The simplest way to show what happens is to just post the patch, so I did that.  The issue primarily involves pointers marked with the dereferenceable metadata/attribute; we used to assume they were naturally aligned, but now we don't.
> 
> Frontends adding dereferenceable attributes/metadata should also add the corresponding alignment attributes/metadata, but I'm not sure that's actually happening consistently in practice.
I still believe the code was incorrect so I'm in favor of removing it. We might need to improve other places but that seems doable. Let's wait for a while to see if others want to chime in. (@fhahn, @lebedev.ri, @spatel, @nikic, @arsenm, @reames, @nlopes)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80072/new/

https://reviews.llvm.org/D80072