[CIR][CIRGen] Support OpenCL Vector Types #613

seven-mile · 2024-05-16T18:33:48Z

Fix #532 .

Support CIRGen of ExtVectorElementExpr that includes swizzle v.xyx and subscription v.s0.

bcardosolopes · 2024-05-18T04:12:15Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

@@ -646,9 +646,69 @@ RValue CIRGenFunction::buildLoadOfLValue(LValue LV, SourceLocation Loc) {

  if (LV.isSimple())
    return RValue::get(buildLoadOfScalar(LV, Loc));
+
+  if (LV.isVectorElt()) {


Seems like this is content from the other patch? If there are dependencies between the PRs, it might be better not to put them up for review just yet, otherwise I have to go to the same thing twice and mentally account for it :)

clang/test/CIR/Lowering/vectype-cl.cpp

clang/test/CIR/CodeGen/vectype-cl.cpp

bcardosolopes

Way to go on the skeleton work and thanks for fixing the testcase, more comments inline.

bcardosolopes · 2024-05-20T18:15:59Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

  llvm_unreachable("NYI");
 }

+int64_t CIRGenFunction::getAccessedFieldNo(unsigned int idx,
+                                           const mlir::Attribute elts) {


Given the uses I've seen elsewhere, elts param could just be a ArrayAttr

bcardosolopes · 2024-05-20T18:17:04Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+  assert(elts.isa<mlir::ArrayAttr>());
+
+  auto elt = elts.cast<mlir::ArrayAttr>()[idx];
+  assert(elt.isa<mlir::IntegerAttr>());


If you are doing a isa followed by cast, better just do dyn_cast and assert if it's null.

bcardosolopes · 2024-05-20T18:18:41Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+  // HLSL allows treating scalars as one-element vectors. Converting the scalar
+  // IR value to a vector here allows the rest of codegen to behave as normal.
+  if (getLangOpts().HLSL && !Vec.getType().isa<mlir::cir::VectorType>()) {
+    auto DstTy =


Paths that are not tested should be llvm_unreachable("NYI");. Unless you are adding HLSL tests, please remove the code within this if statement in favor of llvm_unreachable("NYI");. Please do that for all other places where you didn't add tests to cover those paths.

bcardosolopes · 2024-05-20T18:18:57Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+  // a single element.  Just codegen as an extractelement.
+  const auto *ExprVT = LV.getType()->getAs<clang::VectorType>();
+  if (!ExprVT) {
+    int64_t InIdx = getAccessedFieldNo(0, Elts);


Is this path tested?

bcardosolopes · 2024-05-20T18:21:12Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

@@ -674,6 +727,84 @@ RValue CIRGenFunction::buildLoadOfBitfieldLValue(LValue LV,
  return RValue::get(field);
 }

+void CIRGenFunction::buildStoreThroughExtVectorComponentLValue(RValue Src,
+                                                               LValue Dst) {
+  mlir::Location loc = Dst.getExtVectorPointer().getLoc();


Same for this function, please make sure you add testcases or llvm_unreachable("NYI"); for paths that are not tested.

bcardosolopes · 2024-05-20T18:21:50Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+LValue
+CIRGenFunction::buildExtVectorElementExpr(const ExtVectorElementExpr *E) {
+  // Emit the base vector as an l-value.
+  LValue Base;


Same for this function, please make sure you add testcases or llvm_unreachable("NYI"); for paths that are not tested.

bcardosolopes · 2024-05-20T18:22:27Z

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

+    // If it is a pointer to a vector, emit the address and form an lvalue with
+    // it.
+    LValueBaseInfo BaseInfo;
+    // TODO(cir): Support TBAA


Please also add assert(!UnimplementedFeature::tbaa());

bcardosolopes · 2024-05-20T18:27:03Z

clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp

-  // OpenCL: If the condition is a vector, we can treat this condition like
-  // the select function.
-  if ((CGF.getLangOpts().OpenCL && condExpr->getType()->isVectorType()) ||
-      condExpr->getType()->isExtVectorType()) {


Why are you removing this check? I don't think you have testcases to back this up and doing this will probably lead to some mis-compilation that is going hard to track later. There's specific handling that needs to be done for VisitAbstractConditionalOperator in face of these conditions.

vector_ternary_lowering is on this path. This was mentioned in the PR description (I'll update it to a brief version though):

A special method is used for the original CodeGen for ternary operator of OpenCL vectors. I don't know why. Since we already have high-level cir.vec.ternary, I skipped this part. And the lowering still works well. Maybe it's about the SPIR-V target.

Probably I'll also comment the corresponding part in the PR review ahead of time : )

Since we already have high-level cir.vec.ternary, I skipped this part

Unfortunately that's not a good rationale and not how we usually go about these things in CIR - if this leads to a miscompile, it's going to be really hard to pinpoint the issue, and might take a lot of investigation time. If something was implemented that is shortcuting here, it should also be fixed to maintain the skeleton the much pristine as possible. It's possible you might find out that you need to do another PR to fix that problem before you can land this one.

Alternatively, if you are deleting this code so one of your testcases may pass, you can then choose to not add that testcase and address the whole issue together in another PR.

bcardosolopes · 2024-05-20T18:27:44Z

clang/lib/CIR/CodeGen/CIRGenValue.h

+    assert(isExtVectorElt());
+    return V;
+  }
+  mlir::Attribute getExtVectorElts() const {


From overall usage in this PR, looks like this could return an ArrayAttr instead.

seven-mile marked this pull request as ready for review May 16, 2024 18:38

seven-mile mentioned this pull request May 17, 2024

Fix InitListExpr for OpenCL vectors #615

Open

bcardosolopes requested changes May 18, 2024

View reviewed changes

seven-mile added 2 commits May 18, 2024 13:24

[CIR][CIRGen] Support OpenCL Vector Type

21ad3cd

[CIR][NFC] Arrange test cases

ded43cc

seven-mile force-pushed the opencl-vec-types branch from 7fb1234 to ded43cc Compare May 20, 2024 01:28

bcardosolopes requested changes May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR][CIRGen] Support OpenCL Vector Types #613

[CIR][CIRGen] Support OpenCL Vector Types #613

seven-mile commented May 16, 2024 •

edited

bcardosolopes May 18, 2024

bcardosolopes left a comment

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

bcardosolopes May 20, 2024

seven-mile May 21, 2024

bcardosolopes May 21, 2024

bcardosolopes May 20, 2024

[CIR][CIRGen] Support OpenCL Vector Types #613

Are you sure you want to change the base?

[CIR][CIRGen] Support OpenCL Vector Types #613

Conversation

seven-mile commented May 16, 2024 • edited

Choose a reason for hiding this comment

bcardosolopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seven-mile commented May 16, 2024 •

edited