[Alignment][NFC] Convert MachineIRBuilder::buildDynStackAlloc to Align

[gchatelet]

Summary

The change in IRTranslator is not trivial but is NFC as far as I can tell.

This is patch is part of a series to introduce an Alignment type.
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2019-July/133851.html
See this patch for the introduction of the type: https://reviews.llvm.org/D64790

[gchatelet]

rebase

[courbet]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

1889

I think using a ternary would be more readable.

1895

I'd like to get a knowledgeable pair of eye to look at this. We used to align anything smaller than the stack size to 1, now we align to max(AI.align, preftypealign). This sounds OK, but let's be on the safe side.

[gchatelet]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

1895

@aemerson what do you think?

[gchatelet]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

1895

AFAICT it is not clear what it means use G_DYN_STACKALLOC with an alignment of 0
https://llvm.org/docs/GlobalISel/GenericOpcode.html#g-dyn-stackalloc

@aemerson can you help me understand what's going on here? Or add relevant people to the thread?

[aemerson]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

1895

An alignment of 0 I believe is used to mean there is no alignment requirement at all.

[gchatelet]

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

1895

Ok I dug into it and 0 or 1 have the same meaning.
https://reviews.llvm.org/D77372

For code simplicity, I'll use 1 here.

[gchatelet]

• Address comments and rebase

[courbet]

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

381

What about taking an alignment now that we're clarified the meaning of 0 ? There's only one caller now and no alignment does not have any particular meaning compared to 1.

[gchatelet]

• Using Align instead of MaybeAlign

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

 //===-- llvm/CodeGen/GlobalISel/MachineIRBuilder.h - MIBuilder --*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
public:
   /// Build and insert \p Res = G_DYN_STACKALLOC \p Size, \p Align
   ///
   /// G_DYN_STACKALLOC does a dynamic stack allocation and writes the address of
   /// the allocated memory into \p Res.
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res must be a generic virtual register with pointer type.
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.
   MachineInstrBuilder buildDynStackAlloc(const DstOp &Res, const SrcOp &Size,
-                                         unsigned Align);
+                                         MaybeAlign Align);
+
+  LLVM_ATTRIBUTE_DEPRECATED(inline MachineInstrBuilder buildDynStackAlloc(
+                                const DstOp &Res, const SrcOp &Size,
+                                unsigned Align),
+                            "Use the version that takes MaybeAlign instead") {
+    return buildDynStackAlloc(Res, Size, MaybeAlign(Align));
+  }
 
   /// Build and insert \p Res = G_FRAME_INDEX \p Idx
   ///
   /// G_FRAME_INDEX materializes the address of an alloca value or other
   /// stack-based object.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre \p Res must be a generic virtual register with pointer type.

llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

 //===- llvm/CodeGen/GlobalISel/IRTranslator.cpp - IRTranslator ---*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
bool IRTranslator::translateAlloca(const User &U,
   }
 
   // FIXME: support stack probing for Windows.
   if (MF->getTarget().getTargetTriple().isOSWindows())
     return false;
 
   // Now we're in the harder dynamic case.
   Register NumElts = getOrCreateVReg(*AI.getArraySize());
-
   Type *IntPtrIRTy = DL->getIntPtrType(AI.getType());
   LLT IntPtrTy = getLLTForType(*IntPtrIRTy, *DL);
   if (MRI->getType(NumElts) != IntPtrTy) {
     Register ExtElts = MRI->createGenericVirtualRegister(IntPtrTy);
     MIRBuilder.buildZExtOrTrunc(ExtElts, NumElts);
     NumElts = ExtElts;
   }
 
   Type *Ty = AI.getAllocatedType();
 
   Register AllocSize = MRI->createGenericVirtualRegister(IntPtrTy);
   Register TySize =
       getOrCreateVReg(*ConstantInt::get(IntPtrIRTy, DL->getTypeAllocSize(Ty)));
   MIRBuilder.buildMul(AllocSize, NumElts, TySize);
 
   // Round the size of the allocation up to the stack alignment size
   // by add SA-1 to the size. This doesn't overflow because we're computing
   // an address inside an alloca.
-  unsigned StackAlign =
-      MF->getSubtarget().getFrameLowering()->getStackAlignment();
-  auto SAMinusOne = MIRBuilder.buildConstant(IntPtrTy, StackAlign - 1);
+  Align StackAlign = MF->getSubtarget().getFrameLowering()->getStackAlign();
+  auto SAMinusOne = MIRBuilder.buildConstant(IntPtrTy, StackAlign.value() - 1);
   auto AllocAdd = MIRBuilder.buildAdd(IntPtrTy, AllocSize, SAMinusOne,
                                       MachineInstr::NoUWrap);
   auto AlignCst =
-      MIRBuilder.buildConstant(IntPtrTy, ~(uint64_t)(StackAlign - 1));
+      MIRBuilder.buildConstant(IntPtrTy, ~(uint64_t)(StackAlign.value() - 1));
   auto AlignedAlloc = MIRBuilder.buildAnd(IntPtrTy, AllocAdd, AlignCst);
 
-  unsigned Align =
-      std::max((unsigned)DL->getPrefTypeAlignment(Ty), AI.getAlignment());
-  if (Align <= StackAlign)
-    Align = 0;
-  MIRBuilder.buildDynStackAlloc(getOrCreateVReg(AI), AlignedAlloc, Align);
+  Align Alignment = max(AI.getAlign(), DL->getPrefTypeAlign(Ty));
+  MaybeAlign DynStackAllocAlign = Alignment;
+  if (Alignment <= StackAlign)
+    DynStackAllocAlign = None;
+  MIRBuilder.buildDynStackAlloc(getOrCreateVReg(AI), AlignedAlloc,
+                                DynStackAllocAlign);
 
-  MF->getFrameInfo().CreateVariableSizedObject(Align ? Align : 1, &AI);
+  MF->getFrameInfo().CreateVariableSizedObject(Alignment, &AI);
   assert(MF->getFrameInfo().hasVarSizedObjects());
   return true;
 }
 
 bool IRTranslator::translateVAArg(const User &U, MachineIRBuilder &MIRBuilder) {
   // FIXME: We may need more info about the type. Because of how LLT works,
   // we're completely discarding the i64/double distinction here (amongst
   // others). Fortunately the ABIs I know of where that matters don't use va_arg

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

 //===-- llvm/CodeGen/GlobalISel/MachineIRBuilder.cpp - MIBuilder--*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
assert(cast<DILabel>(Label)->isValidLocationForIntrinsic(State.DL) &&
          "Expected inlined-at fields to agree");
   auto MIB = buildInstr(TargetOpcode::DBG_LABEL);
 
   return MIB.addMetadata(Label);
 }
 
 MachineInstrBuilder MachineIRBuilder::buildDynStackAlloc(const DstOp &Res,
                                                          const SrcOp &Size,
-                                                         unsigned Align) {
+                                                         MaybeAlign Align) {
   assert(Res.getLLTTy(*getMRI()).isPointer() && "expected ptr dst type");
   auto MIB = buildInstr(TargetOpcode::G_DYN_STACKALLOC);
   Res.addDefToMIB(*getMRI(), MIB);
   Size.addSrcToMIB(MIB);
-  MIB.addImm(Align);
+  MIB.addImm(Align ? Align->value() : 0);
   return MIB;
 }
 
 MachineInstrBuilder MachineIRBuilder::buildFrameIndex(const DstOp &Res,
                                                       int Idx) {
   assert(Res.getLLTTy(*getMRI()).isPointer() && "invalid operand type");
   auto MIB = buildInstr(TargetOpcode::G_FRAME_INDEX);
   Res.addDefToMIB(*getMRI(), MIB);