[CIR] Add GEP flags to ptr stride op #1863

badumbatish · 2025-08-26T20:18:57Z

From the comment on PR review #1844, it seems like we're missing the flags for GEP.

I'm opening the PR to add the flags.

The first commit is just a prototype to gather opinions and reviews to see if I'm heading to the right direction with this.

github-actions · 2025-08-26T20:24:03Z

✅ With the latest revision this PR passed the C/C++ code formatter.

bcardosolopes

Nice! This also needs CIRGen changes and lowering to LLVM support, but looks like a good start

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

clang/include/clang/CIR/Dialect/IR/CIROps.td

tommymcm · 2025-08-27T20:13:46Z

It might make sense for cir.get_element to get the same treatment. Maybe we should have it inherit from ptr_stride to avoid duplicating everything? get_element is essentially just a ptr_stride with 'better' type information.

badumbatish · 2025-08-27T20:19:59Z

is there any historical reason why we didn't consolidate the two?

tommymcm · 2025-08-27T20:22:51Z

is there any historical reason why we didn't consolidate the two?

Nope, I introduced get_element recently-ish. It just didn't have any meaningful overlap until this. Other option is having them share a parent (some kind of "ptr_arith" base class).
This can all be handled in a later PR though.

andykaylor · 2025-08-27T20:42:36Z

I like get_element as a separate operation, because it does something logically different than ptr_stride, but if we can share code between them, we should.

badumbatish · 2025-08-27T21:09:10Z

I added lowering and codegen, I added OGCG test initially but was reluctant if i should push it.

I also didn't remove the missing feature check, i'm not sure how feature ready this is compared to OG.

xlauko

Can this be first ported to incubator then mirrored here.

xlauko · 2025-09-02T16:28:02Z

clang/include/clang/CIR/Dialect/IR/CIROps.td

+def GEPInbounds : BitEnumCaseGroup<"inbounds", [GEPInboundsFlag, GEPNusw]>;
+
+def GEPNoWrapFlags
+    : I32BitEnum<"GEPNoWrapFlags", "::cir::GEPNoWrapFlags",


We should add CIR_I32BitEnum similar to CIR_I32Enum.

i'll add a test implement but would love another review on this

Could we just replace these with CIR_I32Enum instead? That way it would match the other enums we use.

i'm more keen on keeping the 1-to-1 mapping between the LLVM dialect and this PR, but i can change if need to

Oh I see, they're fundamentally different types.

clang/include/clang/CIR/Dialect/IR/CIROps.td

xlauko · 2025-09-02T16:34:06Z

clang/include/clang/CIR/Dialect/IR/CIROps.td

+def GEPNoWrapFlagsProp : EnumProp<GEPNoWrapFlags> {
+  let defaultValue = interfaceType#"::none";
+}


Ay reason for property here, and in operation? Named attributes in operations are defaulted to properties, no need to wrap it.

~~this is just copied verbatim from the LLVM dialect, i'll incorporate your comments to the next push~~

Using GEPNoWrapFlags directly resulted in this

/Users/jjasmine/Developer/igalia/clangir/clang/include/clang/CIR/Dialect/IR/CIROps.td:406:5: error: unexpected def type; only defs deriving from TypeConstraint or Attr or Property are allowed def CIR_PtrStrideOp : CIR_Op<"ptr_stride",[ ^

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

xlauko · 2025-09-02T16:41:17Z

clang/include/clang/CIR/Dialect/IR/CIROps.td

+    `(` $base `:` qualified(type($base)) `,` $stride `:` qualified(type($stride))(`,` $noWrapFlags^)?`)`
    `,` qualified(type($result)) attr-dict


This should have been $base, $stride : funstional-type attr-dict in the first place. We can fix that fist in separate PR.

Then I would suggest ($noWrapFlags^)? $base, $stride : funstional-type attr-dict.

yeah i can file an issue for that

badumbatish · 2025-09-04T00:17:44Z

Can this be first ported to incubator then mirrored here.

I dont quite follow, this repo is the incubator yes?

- Add CIR_I32BitEnum similar to CIR_I32Enum. - Add usage example where we use enums - Use look up table instead of static cast when converting from CIR to LLVM

tommymcm

Need to update the CIR-to-LLVM GEP flag function. Also some formatting nits.

tommymcm · 2025-09-04T21:17:41Z

clang/include/clang/CIR/Dialect/IR/CIREnumAttr.td

+class CIR_I32BitEnum<string name, string summary, list<BitEnumCaseBase> cases>
+    : I32BitEnum<name, summary, cases> {


Looking at other dialects, I think this should be:

Suggested change

class CIR_I32BitEnum<string name, string summary, list<BitEnumCaseBase> cases>

: I32BitEnum<name, summary, cases> {

class CIR_I32BitEnumAttr<string name, string summary, list<BitEnumCaseBase> cases>

: I32BitEnumAttr<name, summary, cases> {

tommymcm · 2025-09-04T21:20:53Z

clang/include/clang/CIR/Dialect/IR/CIROps.td

+    %4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds), !cir.ptr<i32>
+
+    %4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds|nuw), !cir.ptr<i32>


Suggested change

%4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds), !cir.ptr<i32>

%4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds|nuw), !cir.ptr<i32>

%5 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds), !cir.ptr<i32>

%6 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds|nuw), !cir.ptr<i32>

tommymcm · 2025-09-04T21:25:01Z

clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+  static std::unordered_map<cir::CIR_GEPNoWrapFlags, mlir::LLVM::GEPNoWrapFlags>
+      mp = {
+          {cir::CIR_GEPNoWrapFlags::none, mlir::LLVM::GEPNoWrapFlags::none},
+          {cir::CIR_GEPNoWrapFlags::inbounds,
+           mlir::LLVM::GEPNoWrapFlags::inbounds},
+          {cir::CIR_GEPNoWrapFlags::inboundsFlag,
+           mlir::LLVM::GEPNoWrapFlags::inboundsFlag},
+          {cir::CIR_GEPNoWrapFlags::nusw, mlir::LLVM::GEPNoWrapFlags::nusw},
+          {cir::CIR_GEPNoWrapFlags::nuw, mlir::LLVM::GEPNoWrapFlags::nuw},
+      };
+  mlir::LLVM::GEPNoWrapFlags x = mlir::LLVM::GEPNoWrapFlags::none;
+  for (auto [key, _] : mp)
+    x = x | mp.at(flags & key);
+  return x;


Don't use a static map like this, it introduces shared state.

This should just be a series of if (flags & cir::CIR_GEP...) x |= mlir::LLVM::GEP...;

Fixes #1888. #1863 requires this to move forward

Fixes llvm#1888. llvm#1863 requires this to move forward

Add prototype of GEP attributes to ptr stride ops

f9c26ce

badumbatish requested review from lanza, bcardosolopes, xlauko and andykaylor as code owners August 26, 2025 20:18

Added necessary header, reformat code

6b1cf57

bcardosolopes requested changes Aug 26, 2025

View reviewed changes

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved

clang/lib/CIR/Dialect/IR/CIRDialect.cpp Show resolved Hide resolved

clang/include/clang/CIR/Dialect/IR/CIROps.td Outdated Show resolved Hide resolved

Reformat to match mlir::LLVM enum

6bbbd84

Implement lowering and codegen, update test case

92b58e8

badumbatish requested a review from bcardosolopes September 2, 2025 04:46

xlauko reviewed Sep 2, 2025

View reviewed changes

This was referenced Sep 4, 2025

[CIR] Update formatting for ptr stride op #1888

Closed

[CIR][NFC] Update formatting for ptr stride op #1889

Merged

Resolve PR reviews

da7c2af

- Add CIR_I32BitEnum similar to CIR_I32Enum. - Add usage example where we use enums - Use look up table instead of static cast when converting from CIR to LLVM

tommymcm reviewed Sep 4, 2025

View reviewed changes

bcardosolopes pushed a commit that referenced this pull request Sep 5, 2025

[CIR][NFC] Update formatting for ptr stride op (#1889)

29b2a4e

Fixes #1888. #1863 requires this to move forward

tommymcm pushed a commit to tommymcm/clangir that referenced this pull request Sep 5, 2025

[CIR][NFC] Update formatting for ptr stride op (llvm#1889)

7837d47

Fixes llvm#1888. llvm#1863 requires this to move forward

		`(` $base `:` qualified(type($base)) `,` $stride `:` qualified(type($stride))(`,` $noWrapFlags^)?`)`
		`,` qualified(type($result)) attr-dict

		class CIR_I32BitEnum<string name, string summary, list<BitEnumCaseBase> cases>
		: I32BitEnum<name, summary, cases> {

		%4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds), !cir.ptr<i32>

		%4 = cir.ptr_stride(%2 : !cir.ptr<i32>, %3 : i32, inbounds\|nuw), !cir.ptr<i32>

[CIR] Add GEP flags to ptr stride op #1863

Are you sure you want to change the base?

[CIR] Add GEP flags to ptr stride op #1863

Uh oh!

Conversation

badumbatish commented Aug 26, 2025

Uh oh!

github-actions bot commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardosolopes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tommymcm commented Aug 27, 2025

Uh oh!

badumbatish commented Aug 27, 2025

Uh oh!

tommymcm commented Aug 27, 2025

Uh oh!

andykaylor commented Aug 27, 2025

Uh oh!

badumbatish commented Aug 27, 2025

Uh oh!

xlauko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badumbatish Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badumbatish Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badumbatish commented Sep 4, 2025

Uh oh!

tommymcm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 26, 2025 •

edited

Loading

badumbatish Sep 4, 2025 •

edited

Loading

badumbatish Sep 4, 2025 •

edited

Loading