barannikov88 added inline comments.
================
Comment at: clang/lib/CodeGen/CGCUDANV.cpp:491
new llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,
- llvm::Align(Var->getAlignment()), I);
+ Var->getAlign().valueOrOne(), I);
WorkItem.pop_back();
----------------
tra wrote:
> gchatelet wrote:
> > tra wrote:
> > > This appears to be a change in behavior. AFAICT, previously used
> > > Var->getAlignment() could return alignment value or 0. Now it's value or
> > > 1.
> > >
> > > Is it intentional?
> > The previous statement was constructing an `llvm::Align` from a value, and
> > `llvm::Align` [asserts when the provided value is
> > 0](https://github.com/llvm/llvm-project/blob/4ab2246d486ba30c4b2d654323a0c0b97565c0f1/llvm/include/llvm/Support/Alignment.h#L76-L81).
> > This means that it is undefined to pass the value `0`.
> >
> > As far as `LoadInst` is concerned it can only accept valid alignments
> > (i.e., powers of two => non zero).
> >
> > So you're right that it is not strictly NFC and that
> > `*Var->getAlign()`would be a more rigorous transformation but I //think//
> > that converting the `0` value to `1` moves us from UB to semantically valid
> > code.
> >
> > I don't feel strongly about it though and I'm fine changing this to
> > `*Var->getAlign()` to keep this patch NFC. WDYT?
> Enforcing alignment of 1 would potentially force us to generate overly
> conservative one byte at a time loads/stores.
> I agree that passing 0 is a wrong choice here, but 1 does not seem to be
> correct, either.
> Unfortunately LoadInst does not have overloads accepting MaybeAlign so we
> need to use different `LoadInst` overload depending on whether alignment is
> specified.
>
> ```
> NewV = Var->getAlign().isAligned()
> ? llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false,
> Var->getAlign().value(), I)
> : llvm::LoadInst(Var->getType(), ManagedVar, "ld.managed", false, I);
> ```
>
> @yaxunl -- Sam, does it make sense? This seems to be largely HIP-specific.
I think it should be just `Var->getAlign()` to allow propagating absent
alignment.
Curiously, the old code didn't assert because `GlobalVariable` seem to
always(?) have non-empty alignment if the global had an associated `VarDecl`
(set [[
https://github.com/llvm/llvm-project/blob/6ad0788c332bb2043142954d300c49ac3e537f34/clang/lib/CodeGen/CodeGenModule.cpp#L4442
| here ]], changed(?) by [[
https://github.com/llvm/llvm-project/commit/c79099e0f44d0f85515fd30c83923d9d9dc1679b
| this patch ]]).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142459/new/
https://reviews.llvm.org/D142459
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
