[Verifier] Verify parameter alignment.

[LuoYuanke]

Summary

In DAGISel, the parameter alignment only have 4 bits to hold the value.
The encode(alignment) would need extra 1 bit, so the max aligment that
ISel can support is 2^14. This patch verify the parameter, return value
and align attribute for parameter.

[skan]

llvm/lib/IR/Verifier.cpp

1749

A question: why is the max alignment is not 1<<15 if we have 4 bits?

3159

This is a little weird. We hard-code the max number by the width of a bit field...

[skan]

Is it possible that we define a common max alignment across all layers?

[pengfei]

In D121898#3391242, @skan wrote:

Is it possible that we define a common max alignment across all layers?

I think arbitrary/maximum alignment doesn't make sense in reality and usually implies something wrong. So adding a verifier seems a good choice. LGTM but wait one or two days for other reviewers.

[skan]

llvm/lib/IR/Verifier.cpp

3156

At least we need to add comments at ArgFlagsTy::MemAlign to say that we verify the width of bit field in Verify.cpp.

[LuoYuanke]

In D121898#3391242, @skan wrote:

Is it possible that we define a common max alignment across all layers?

After digging more in the LLVM code, I find there is MaximumAlignment (1 << 32) in Value.h (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Value.h#L793) and constant null pointer would be set the MaximumAlignment at https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Value.cpp#L977. Below test case would fail with my patch which constrain parameter alignment. To fix the max size conflict between MaximumAlignment and ArgFlagsTy.MemAlign maybe just use unsigned type for MemAlign instead of bitfield? But it seems there is assert to monitor ArgFlagsTy size at https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/TargetCallingConv.h#L69.

So in summary, for now I have no idea how to fix it. Maybe leave the test case of my patch just crash during the lowering and remind middle-end generate the right vector size or alignment attribute. @craig.topper, @RKSimon, @pengfei any ideas?

; RUN: opt -passes=instcombine -S < %s | FileCheck %s

define i32 @PR48810() {
; CHECK-LABEL: @PR48810(
; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 undef, i8* align 4294967296 null, i64 undef, i1 false)
; CHECK-NEXT:    ret i32 undef
;
  %r = call dereferenceable(1) i8* @mempcpy(i8* undef, i8* null, i64 undef)
  ret i32 undef
}

declare i8* @mempcpy(i8*, i8* nocapture readonly, i64)

[pengfei]

Can we check if the pointer has byval/byref attribute? I think the MemAlign should only be meaningful for them. And if we simply passing a pointer with OrigAlign, it's safe to truncate to the maximum of MemAlign in backend.

[pengfei]

Further more, byval/byref together with align attribute are all ABI related. It does make sense to have different alignment with a pure pointer used inside function only. Here's comment from LangRef:

The byval attribute also supports specifying an alignment with the align attribute. It indicates the alignment of the stack slot to form and the known alignment of the pointer specified to the call site. If the alignment is not specified, then the code generator makes a target-specific assumption.

[skan]

In D121898#3392126, @LuoYuanke wrote:

In D121898#3391242, @skan wrote:

Is it possible that we define a common max alignment across all layers?

After digging more in the LLVM code, I find there is MaximumAlignment (1 << 32) in Value.h (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/Value.h#L793) and constant null pointer would be set the MaximumAlignment at https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Value.cpp#L977. Below test case would fail with my patch which constrain parameter alignment. To fix the max size conflict between MaximumAlignment and ArgFlagsTy.MemAlign maybe just use unsigned type for MemAlign instead of bitfield? But it seems there is assert to monitor ArgFlagsTy size at https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/TargetCallingConv.h#L69.

So in summary, for now I have no idea how to fix it. Maybe leave the test case of my patch just crash during the lowering and remind middle-end generate the right vector size or alignment attribute. @craig.topper, @RKSimon, @pengfei any ideas?

; RUN: opt -passes=instcombine -S < %s | FileCheck %s

define i32 @PR48810() {
; CHECK-LABEL: @PR48810(
; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 undef, i8* align 4294967296 null, i64 undef, i1 false)
; CHECK-NEXT:    ret i32 undef
;
  %r = call dereferenceable(1) i8* @mempcpy(i8* undef, i8* null, i64 undef)
  ret i32 undef
}

declare i8* @mempcpy(i8*, i8* nocapture readonly, i64)

Could we add the max alignment information to Triple?

[LuoYuanke]

1. Remove the alignment attribute check. I plan to do it in another patch.
2. Rebase.

[LuoYuanke]

Address Shengchen's comments.

[LuoYuanke]

In D121898#3392260, @pengfei wrote:

Can we check if the pointer has byval/byref attribute? I think the MemAlign should only be meaningful for them. And if we simply passing a pointer with OrigAlign, it's safe to truncate to the maximum of MemAlign in backend.

LGTM, thanks Phoebe. I create anther patch at https://reviews.llvm.org/D122130.

[pengfei]

llvm/lib/IR/Verifier.cpp

3155–3163

The variables are only used in Assert, should add #ifndef NDEBUG to them? Surprisingly I didn't find an example in this file.

llvm/test/Verifier/param-ret-align.ll

2

Can we merge them to one file?

[LuoYuanke]

llvm/lib/IR/Verifier.cpp

3155–3163

The Assert is defined as below. Let me check if it will fail in release build.

#define Assert(C, ...) \
  do { if (!(C)) { CheckFailed(__VA_ARGS__); return; } } while (false)

llvm/test/Verifier/param-ret-align.ll

2

Probably not. llvm-as would exit on the first failure.

[skan]

llvm/test/Verifier/param-ret-align.ll

2

Then? We could check more than one failure in a LIT test.

[skan]

llvm/lib/IR/Verifier.cpp

3155–3163

This Assert is different from assert. It is enabled no matter NDEBUG is turned on or off

[LuoYuanke]

Update comments.

[pengfei]

Still fine to me.

llvm/test/Verifier/param-ret-align.ll

2

I understand it is the second function will never be handled once the first failed. With a rough search, I didn't find an example to handle more than one fucntions.

[pengfei]

llvm/lib/IR/Verifier.cpp

3155–3163

Good to know, thanks!

[skan]

llvm/test/Verifier/param-ret-align.ll

2

Oh, I understand what yuanke means now

[LuoYuanke]

Rebase and have constant for parameter max alignment.

[skan]

LGTM now.

[nikic]

Just to throw it out there, we should be able to support a larger alignment in ArgFlagsTy pretty easily, because PointerAddrSpace only needs 24 bits, so we have 8 unused bits still left in that structure.

[LuoYuanke]

In D121898#3399319, @nikic wrote:

Just to throw it out there, we should be able to support a larger alignment in ArgFlagsTy pretty easily, because PointerAddrSpace only needs 24 bits, so we have 8 unused bits still left in that structure.

Looks good. Is there any document for the bit size of PointerAddrSpace?

[nikic]

@LuoYuanke Not sure if this is documented anywhere, but there is this check in the DataLayout implementation: https://github.com/llvm/llvm-project/blob/fea20cb99087208d589d81bb7bbbc84198c7ffa4/llvm/lib/IR/DataLayout.cpp#L250-L256

[LuoYuanke]

In D121898#3399677, @nikic wrote:

@LuoYuanke Not sure if this is documented anywhere, but there is this check in the DataLayout implementation: https://github.com/llvm/llvm-project/blob/fea20cb99087208d589d81bb7bbbc84198c7ffa4/llvm/lib/IR/DataLayout.cpp#L250-L256

Thanks, @nikic. I'll come up with a patch to enlarge the alignment bit field.

[skan]

In D121898#3401216, @LuoYuanke wrote:

In D121898#3399677, @nikic wrote:

@LuoYuanke Not sure if this is documented anywhere, but there is this check in the DataLayout implementation: https://github.com/llvm/llvm-project/blob/fea20cb99087208d589d81bb7bbbc84198c7ffa4/llvm/lib/IR/DataLayout.cpp#L250-L256

Thanks, @nikic. I'll come up with a patch to enlarge the alignment bit field.

I would argue that maybe we should shrink the alignment bit field...

[pengfei]

Yeah, I don't think it's worth to add one more bit just for avoiding error. We don't have the requirment to store such a large alignment. The OrigAlign is another topic, I think we can leave it as is.

[pirama]

In D121898#3401296, @skan wrote:

In D121898#3401216, @LuoYuanke wrote:

In D121898#3399677, @nikic wrote:

@LuoYuanke Not sure if this is documented anywhere, but there is this check in the DataLayout implementation: https://github.com/llvm/llvm-project/blob/fea20cb99087208d589d81bb7bbbc84198c7ffa4/llvm/lib/IR/DataLayout.cpp#L250-L256

Thanks, @nikic. I'll come up with a patch to enlarge the alignment bit field.

I would argue that maybe we should shrink the alignment bit field...

As seen in https://github.com/llvm/llvm-project/issues/58304, some intrinsics need a parameter with higher alignment (2 << 15). Should we extend the alignment field to accommodate that?

[LuoYuanke]

This comment was removed by LuoYuanke.

[LuoYuanke]

As seen in https://github.com/llvm/llvm-project/issues/58304, some intrinsics need a parameter with higher alignment (2 << 15). Should we extend the alignment field to accommodate that?

Maybe we can, but it seems an infrastructure change. Not sure what the risk we'd take for the change.

[dmgreen]

In the case of #58304 it is a @llvm.vector.reduce.or.v16i8488(<16 x i8488> %18) that is incorrectly failing, which is not a real call. Could we exclude intrinsics from this verifier check?

[LuoYuanke]

In D121898#3870219, @dmgreen wrote:

In the case of #58304 it is a @llvm.vector.reduce.or.v16i8488(<16 x i8488> %18) that is incorrectly failing, which is not a real call. Could we exclude intrinsics from this verifier check?

It sounds reasonable to me. Let me create a patch to exclude intrinsics.

[LuoYuanke]

I remove the constrain for intrinsics with https://reviews.llvm.org/D136330. However when testing below intrinsics case, it still crashes on DAGISel.

$ cat param-align.ll
define dso_local float @foo(<8192 x float> noundef %vec) {
entry:
  %b = call float @llvm.vector.reduce.fadd.f32.v8192f32(float -0.0, <8192 x float> %vec)
  ret float %b
}

declare float @llvm.vector.reduce.fadd.f32.v8192f32(float %start_value, <8192 x float> %a)
$ llc param-align.ll
llc: /export/users/workspace/opensrc/llvm-project-github/llvm/include/llvm/CodeGen/TargetCallingConv.h:150: void llvm::ISD::ArgFlagsTy::setMemAlign(llvm::Align): Assertion `getNonZeroMemAlign() == A && "bitfield overflow"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

[dmgreen]

In D121898#3870522, @LuoYuanke wrote:

I remove the constrain for intrinsics with https://reviews.llvm.org/D136330. However when testing below intrinsics case, it still crashes on DAGISel.

$ cat param-align.ll
define dso_local float @foo(<8192 x float> noundef %vec) {
entry:
  %b = call float @llvm.vector.reduce.fadd.f32.v8192f32(float -0.0, <8192 x float> %vec)
  ret float %b
}

declare float @llvm.vector.reduce.fadd.f32.v8192f32(float %start_value, <8192 x float> %a)
$ llc param-align.ll
llc: /export/users/workspace/opensrc/llvm-project-github/llvm/include/llvm/CodeGen/TargetCallingConv.h:150: void llvm::ISD::ArgFlagsTy::setMemAlign(llvm::Align): Assertion `getNonZeroMemAlign() == A && "bitfield overflow"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

That is presumably because of the <8192 x float> noundef %vec argument for @foo. If it was loaded from a pointer, does it still fail? It will be a pretty huge expansion, but should work OK.

[LuoYuanke]

That is presumably because of the <8192 x float> noundef %vec argument for @foo. If it was loaded from a pointer, does it still fail? It will be a pretty huge expansion, but should work OK.

It doesn't fail when <8192 x float> %vec is loaded from a pointer.

llvm/include/llvm/CodeGen/TargetCallingConv.h

private:
     unsigned IsSwiftSelf : 1;  ///< Swift self parameter
     unsigned IsSwiftAsync : 1;  ///< Swift async context parameter
     unsigned IsSwiftError : 1; ///< Swift error parameter
     unsigned IsCFGuardTarget : 1; ///< Control Flow Guard target
     unsigned IsHva : 1;        ///< HVA field for
     unsigned IsHvaStart : 1;   ///< HVA structure start
     unsigned IsSecArgPass : 1; ///< Second argument
     unsigned MemAlign : 4;     ///< Log 2 of alignment when arg is passed in memory
-                               ///< (including byval/byref)
+                               ///< (including byval/byref). The max alignment is
+                               ///< verified in IR verification.
     unsigned OrigAlign : 5;    ///< Log 2 of original alignment
     unsigned IsInConsecutiveRegsLast : 1;
     unsigned IsInConsecutiveRegs : 1;
     unsigned IsCopyElisionCandidate : 1; ///< Argument copy elision candidate
     unsigned IsPointer : 1;
 
     unsigned ByValOrByRefSize = 0; ///< Byval or byref struct size
 

llvm/lib/IR/Verifier.cpp

 
 } // namespace llvm
 
 namespace {
 
 class Verifier : public InstVisitor<Verifier>, VerifierSupport {
   friend class InstVisitor<Verifier>;
 
+  // ISD::ArgFlagsTy::MemAlign only have 4 bits for alignment, so
+  // the alignment size should not exceed 2^15. Since encode(Align)
+  // would plus the shift value by 1, the alignment size should
+  // not exceed 2^14, otherwise it can NOT be properly lowered
+  // in backend.
+  static constexpr unsigned ParamMaxAlignment = 1 << 14;
   DominatorTree DT;
 
   /// When verifying a basic block, keep track of all of the
   /// instructions we have seen so far.
   ///
   /// This allows us to do efficient dominance checks for the case when an
   /// instruction has an operand that is an instruction in the same block.
   SmallPtrSet<Instruction *, 16> InstsInThisBlock;
void Verifier::verifyParameterAttrs(AttributeSet Attrs, Type *Ty,
   for (Attribute Attr : Attrs)
     Assert(Attr.isStringAttribute() ||
            Attribute::canUseAsParamAttr(Attr.getKindAsEnum()),
            "Attribute '" + Attr.getAsString() +
                "' does not apply to parameters",
            V);
 
   if (Attrs.hasAttribute(Attribute::ImmArg)) {
     Assert(Attrs.getNumAttributes() == 1,
            "Attribute 'immarg' is incompatible with other attributes", V);
   }
 
   // Check for mutually incompatible attributes.  Only inreg is compatible with
   // sret.
   unsigned AttrCount = 0;
   AttrCount += Attrs.hasAttribute(Attribute::ByVal);
   AttrCount += Attrs.hasAttribute(Attribute::InAlloca);
Assert(Call.getArgOperand(i)->getType() == FTy->getParamType(i),
            "Call parameter type does not match function signature!",
            Call.getArgOperand(i), FTy->getParamType(i), Call);
 
   AttributeList Attrs = Call.getAttributes();
 
   Assert(verifyAttributeCount(Attrs, Call.arg_size()),
          "Attribute after last parameter!", Call);
 
+  auto VerifyTypeAlign = [&](Type *Ty, const Twine &Message) {
+    if (!Ty->isSized())
+      return;
+    Align ABIAlign = DL.getABITypeAlign(Ty);
+    Align MaxAlign(ParamMaxAlignment);
+    Assert(ABIAlign < MaxAlign,
+           "Incorrect alignment of " + Message + " to called function!", Call);
+  };
+
+  VerifyTypeAlign(FTy->getReturnType(), "return type");
+  for (unsigned i = 0, e = FTy->getNumParams(); i != e; ++i) {
+    Type *Ty = FTy->getParamType(i);
+    VerifyTypeAlign(Ty, "argument passed");
+  }
+
   Function *Callee =
       dyn_cast<Function>(Call.getCalledOperand()->stripPointerCasts());
   bool IsIntrinsic = Callee && Callee->isIntrinsic();
   if (IsIntrinsic)
     Assert(Callee->getValueType() == FTy,
            "Intrinsic called with incompatible signature", Call);
 
   if (Attrs.hasFnAttr(Attribute::Speculatable)) {

llvm/test/Verifier/param-align.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+; CHECK: Incorrect alignment of argument passed to called function!
+define dso_local void @foo(<8192 x float> noundef %vec) {
+entry:
+  call void @bar(<8192 x float> %vec)
+  ret void
+}
+
+declare dso_local void @bar(<8192 x float>)

llvm/test/Verifier/param-ret-align.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+; CHECK: Incorrect alignment of return type to called function!
+define dso_local void @foo() {
+entry:
+  call <8192 x float> @bar()
+  ret void
+}
+
+declare dso_local <8192 x float> @bar()