arsenm 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();
----------------
yaxunl wrote:
> efriedma wrote:
> > gchatelet wrote:
> > > barannikov88 wrote:
> > > > barannikov88 wrote:
> > > > > 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 ]]).
> > > > >
> > > > > Unfortunately LoadInst does not have overloads accepting MaybeAlign
> > > > > so we need to use different `LoadInst` overload depending on whether
> > > > > alignment is specified.
> > > >
> > > > That's interesting, because `SelectionDAG::getLoad` has many variants
> > > > with `MaybeAlign`, but only one with `Align`.
> > > >
> > > [The documentation](https://llvm.org/docs/LangRef.html#load-instruction)
> > > says that `LoadInst` alignment is optional so indeed it should accept a
> > > `MaybeAlign` instead of an `Align`.
> > > I'll send a patch to fix this.
> > >
> > > Then we can indeed simply use `Var->getAlign()`.
> > Alignment is "optional" in textual IR for backward-compatibility/ease of
> > use... but it's just a shortcut for specifying "use the ABI alignment of
> > the value type". It isn't optional in memory. Maybe LangRef could be
> > updated to make that a bit more clear. (See D77454.)
> >
> > We should consider doing something similar for globals, but at the time, I
> > ran out of energy to try to deal with that; backend handling for globals
> > with unspecified alignment is weird.
> > 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.
>
> Thanks for reminding me about this.
>
> If it is 0 it should have caused an assertion.
>
> We can probably use getAlign().value() to keep the original asserting
> behaviour instead of silently using value 1.
>
>
I think MaybeAlign is ridiculous and we should just require alignments
everywhere. It's never really optional, it's just hidden sometimes
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
