[Clang][MVE] Use IRBuilder methods to emit masked load/store (NFC)

[nikic]

In preparation for an upcoming change which will convert the alignment argument to an alignment attribute, switch MVE builtin codegen to use the IRBuilder methods (which will handle this transparently) instead of explicitly emitting the intrinsic.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nikita Popov (nikic)

Changes

In preparation for an upcoming change which will convert the alignment argument to an alignment attribute, switch MVE builtin codegen to use the IRBuilder methods (which will handle this transparently) instead of explicitly emitting the intrinsic.

---

Full diff: https://github.com/llvm/llvm-project/pull/163790.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/arm_mve.td (+8-16)
• (modified) clang/include/clang/Basic/arm_mve_defs.td (+7)
• (modified) clang/utils/TableGen/MveEmitter.cpp (+2-1)

diff --git a/clang/include/clang/Basic/arm_mve.td b/clang/include/clang/Basic/arm_mve.td
index 412ef9abac1bc..2e5e1d93be096 100644
--- a/clang/include/clang/Basic/arm_mve.td
+++ b/clang/include/clang/Basic/arm_mve.td
@@ -831,9 +831,8 @@ multiclass contiguous_load<string mnemonic, PrimitiveType memtype,
          NameOverride<mnemonic>;
     def: Intrinsic<Vector, (args CPtr<CopyKind<same_size[0], Scalar>>:$addr,
                                  Predicate:$pred),
-                   (IRIntBase<"masked_load", [Vector, CPtr<Vector>]>
-                        (CPtr<Vector> $addr), !srl(memtype.size,3),
-                        $pred, (zeroinit Vector))>,
+                   (masked_load Vector, (CPtr<Vector> $addr),
+                       !srl(memtype.size,3), $pred, (zeroinit Vector))>,
          NameOverride<mnemonic # "_z">;
   }
 
@@ -846,9 +845,8 @@ multiclass contiguous_load<string mnemonic, PrimitiveType memtype,
          NameOverride<"vld1q">;
     def: Intrinsic<Vector, (args CPtr<CopyKind<same_size[0], Scalar>>:$addr,
                                  Predicate:$pred),
-                   (IRIntBase<"masked_load", [Vector, CPtr<Vector>]>
-                        (CPtr<Vector> $addr), !srl(memtype.size,3),
-                        $pred, (zeroinit Vector))>,
+                   (masked_load Vector, (CPtr<Vector> $addr),
+                        !srl(memtype.size,3), $pred, (zeroinit Vector))>,
          NameOverride<"vld1q_z">;
   }
 
@@ -863,9 +861,7 @@ multiclass contiguous_load<string mnemonic, PrimitiveType memtype,
          NameOverride<mnemonic>;
     def: Intrinsic<Vector, (args CPtr<CopyKind<same_size[0], Scalar>>:$addr,
                                  Predicate:$pred),
-                   (extend (IRIntBase<"masked_load",
-                                      [NarrowedVecOf<memtype,Vector>,
-                                      CPtr<NarrowedVecOf<memtype,Vector>>]>
+                   (extend (masked_load NarrowedVecOf<memtype,Vector>,
                                 (CPtr<NarrowedVecOf<memtype,Vector>> $addr),
                                 !srl(memtype.size,3), $pred,
                                 (zeroinit NarrowedVecOf<memtype,Vector>)),
@@ -890,8 +886,7 @@ multiclass contiguous_store<string mnemonic, PrimitiveType memtype,
          NameOverride<mnemonic>;
     def: Intrinsic<Void, (args Ptr<CopyKind<same_size[0], Scalar>>:$addr,
                                Vector:$value, Predicate:$pred),
-                   (IRIntBase<"masked_store", [Vector, Ptr<Vector>]>
-                        $value, (Ptr<Vector> $addr),
+                   (masked_store $value, (Ptr<Vector> $addr),
                         !srl(memtype.size,3), $pred)>,
          NameOverride<mnemonic # "_p">;
   }
@@ -907,8 +902,7 @@ multiclass contiguous_store<string mnemonic, PrimitiveType memtype,
          NameOverride<"vst1q">;
     def: Intrinsic<Void, (args Ptr<CopyKind<same_size[0], Scalar>>:$addr,
                                Vector:$value, Predicate:$pred),
-                   (IRIntBase<"masked_store", [Vector, Ptr<Vector>]>
-                        $value, (Ptr<Vector> $addr),
+                   (masked_store $value, (Ptr<Vector> $addr),
                         !srl(memtype.size,3), $pred)>,
          NameOverride<"vst1q_p">;
   }
@@ -925,9 +919,7 @@ multiclass contiguous_store<string mnemonic, PrimitiveType memtype,
          NameOverride<mnemonic>;
     def: Intrinsic<Void, (args Ptr<CopyKind<same_size[0], Scalar>>:$addr,
                                Vector:$value, Predicate:$pred),
-                   (IRIntBase<"masked_store",
-                              [NarrowedVecOf<memtype,Vector>,
-                               Ptr<NarrowedVecOf<memtype,Vector>>]>
+                   (masked_store
                         (trunc $value, NarrowedVecOf<memtype,Vector>),
                         (Ptr<NarrowedVecOf<memtype,Vector>> $addr),
                         !srl(memtype.size,3), $pred)>,
diff --git a/clang/include/clang/Basic/arm_mve_defs.td b/clang/include/clang/Basic/arm_mve_defs.td
index 083d03a396ba3..c1562a0c1f04c 100644
--- a/clang/include/clang/Basic/arm_mve_defs.td
+++ b/clang/include/clang/Basic/arm_mve_defs.td
@@ -134,6 +134,13 @@ def unzip: CGHelperFn<"VectorUnzip"> {
 }
 def zip: CGHelperFn<"VectorZip">;
 
+def masked_load: IRBuilder<"CreateMaskedLoad"> {
+  let special_params = [IRBuilderIntParam<2, "Align">];
+}
+def masked_store: IRBuilder<"CreateMaskedStore"> {
+  let special_params = [IRBuilderIntParam<2, "Align">];
+}
+
 // Trivial 'codegen' function that just returns its argument. Useful
 // for wrapping up a variable name like $foo into a thing you can pass
 // around as type 'dag'.
diff --git a/clang/utils/TableGen/MveEmitter.cpp b/clang/utils/TableGen/MveEmitter.cpp
index a003b5e632d58..f55a5f54bd158 100644
--- a/clang/utils/TableGen/MveEmitter.cpp
+++ b/clang/utils/TableGen/MveEmitter.cpp
@@ -1684,7 +1684,8 @@ void EmitterBase::EmitBuiltinCG(raw_ostream &OS) {
         OS << "  case ARM::BI__builtin_arm_" << OI.Int->builtinExtension()
            << "_" << OI.Name << ":\n";
         for (size_t i = 0, e = MG.ParamTypes.size(); i < e; ++i)
-          OS << "    Param" << utostr(i) << " = " << OI.ParamValues[i] << ";\n";
+          OS << "    Param" << utostr(i) << " = static_cast<"
+             << MG.ParamTypes[i] << ">(" << OI.ParamValues[i] << ");\n";
         OS << "    break;\n";
       }
       OS << "  }\n";

[statham-arm]

The commit message doesn't explain why this change is needed, and I can't work it out by myself. What goes wrong without it?

[nikic]

The Align constructor is explicit, so it's not possible to directly assign an integer to an Align variable.

[statham-arm]

I'd guessed that much, but I was still confused about why the cast there, when the static_cast<Align> was already present in the actual call to Builder.CreateMaskedStore.

But now I've looked at the output of your modified clang-tblgen, I understand:

case ARM::BI__builtin_arm_mve_vst1q_p_f16:
// ...
case ARM::BI__builtin_arm_mve_vstrwq_p_u32: {
  llvm::Type * Param0;
  Align  Param1;
  switch (BuiltinID) {
  case ARM::BI__builtin_arm_mve_vst1q_p_f16:
    Param0 = static_cast<llvm::Type *>(llvm::FixedVectorType::get(Builder.getInt1Ty(), 8));
    Param1 = static_cast<Align>(2);
    break;
    // ...
  case ARM::BI__builtin_arm_mve_vstrwq_p_u32:
    Param0 = static_cast<llvm::Type *>(llvm::FixedVectorType::get(Builder.getInt1Ty(), 4));
    Param1 = static_cast<Align>(4);
    break;
  }
  // ...
  return Builder.CreateMaskedStore(Val0, Val2, static_cast<Align>(Param1), Val5);
}

If the cast in the CreateMaskedStore were to be enough, it would have to be because Param1 had some other type which could be initialized via an integer literal, and it's easier to add a second cast to the same type than to invent an appropriate alternative type.

Sorry about being slow to get the point!