[PATCH] D99037: [Matrix] Implement explicit type conversions for matrix types

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:547
+      case CK_MatrixCast: {
+        // TODO: Handle MatrixCast here.
+      }
----------------
SaurabhJha wrote:
> I thought doing changes here is is outside the scope of casting so I just left a TODO here. Please let me know if we want to do something else here.
I think that's fair,  but you should probably add `continue;` rather than falling through.


================
Comment at: clang/test/CodeGen/matrix-cast.c:15
+  // CHECK:       [[C:%.*]] = load <25 x i8>, <25 x i8>* {{.*}}, align 1
+  // CHECK-NEXT:  [[CONV:%.*]] = zext <25 x i8> [[C]] to <25 x i32>
+  // CHECK-NEXT:  store <25 x i32> [[CONV]], <25 x i32>* {{.*}}, align 4
----------------
This doesn't seem right. We are casting between 2 signed types, so the sign should get preserved, right? Shouldn't this be `sext`? See https://godbolt.org/z/zWznYdnKW for the scalar case.

I think you also need tests for casts with different bitwidths with unsigned, unsigned -> signed & signed -> unsigned.


================
Comment at: clang/test/CodeGen/matrix-cast.c:39
+
+  f = (fx5x5)i;
+}
----------------
SaurabhJha wrote:
> I tried adding a float -> int conversion too but it failed because of this assertion https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGExprScalar.cpp#L1339-L1344 Hopefully that's intended.
Clang should never run into an assertion. If that should not be allowed, then Clang should emit an error during Sema. I'm not sure why there is such a restriction for vector types, but I am not sure that this shouldn't be allowed for matrixes. Perhaps @rjmccall has more thoughts on this, but conversion between scalar ints to floats is allowed AFAIKT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99037