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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits