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

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked an inline comment as done.
efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/Loads.cpp:40
+      BA = DL.getABITypeAlign(Ty);
   }
+  const APInt APAlign(Offset.getBitWidth(), Alignment.value());
----------------
efriedma wrote:
> nikic wrote:
> > jdoerfert wrote:
> > > 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)
> > Agree that the current behavior is incorrect and we really need to do something about this, because it will block opaque pointers as well.
> > 
> > I think Eli is right though and Clang doesn't emit the necessary alignment attributes here right now, so that probably needs to be addressed first.
> I think D80166 takes care of clang.  I can't find any other code that needs to be updated.
Just merged D80166.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80072