[PATCH] D80716: [AArch64]: BFloat Load/Store Intrinsics&CodeGen

[Luke Geeson via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:10366
+    auto Alignment = CGM.getNaturalPointeeTypeAlignment(
+        E->getArg(0)->IgnoreParenCasts()->getType());
     Ops[0] = Builder.CreateBitCast(Ops[0], llvm::PointerType::getUnqual(VTy));
----------------
pratlucas wrote:
> LukeGeeson wrote:
> > simon_tatham wrote:
> > > What effect is this change of strategy having on the alignment computation, for the already-supported instances of this builtin?
> > > 
> > > It //looks// to me as if `__builtin_neon_vld1_v` with (say) a `uint8_t *` pointer argument will now compute `Alignment=1` (the natural alignment of the pointee type), whereas it would previously have computed `Alignment=8` (the size of the whole vector being loaded or stored).
> > > 
> > > Is that intentional? Or accidental? Or have I completely misunderstood what's going on?
> > > 
> > > (Whichever of the three, some discussion in the commit message would be a good idea, explaining why this change does or does not make a difference, as appropriate.)
> > Clang was incorrectly assuming that all the pointers from which loads were being generated for vld1 intrinsics were aligned according to according to the intrinsics result type. This causes alignment faults on the code generated by the backend.
> > This fixes the issue so that alignment is based on the type of the pointer provided to as input to the intrinsic.
> >  
> > @pratlucas has done some work on this in parallel https://reviews.llvm.org/D79721 which has been approved and may overrule this particular line of code. 
> > 
> > I shall add a note to the commit message, and tentatively mark this as fixed, given it's liable to adopt the work of Lucas.
> > 
> Hi @LukeGeeson ,
> Just as a heads up, some changes to this implementations were requested on D79721.
> The usage of `CGM.getNaturalPointeeTypeAlignment` and `IgnoreParenCasts()` was causing problems on certain argument types, so the alignment is now captured from the expression itself when it is emitted.
Thanks @pratlucas yeah I plan to rebase my code onto upstream when you have made those changes (and fix whatever still breaks) before I push :) 


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

https://reviews.llvm.org/D80716