[PATCH] D142459: [clang] Deprecate uses of GlobalObject::getAlignment

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet 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();
----------------
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()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142459