Skip to content

Conversation

@tommymcm
Copy link
Collaborator

@tommymcm tommymcm commented Sep 5, 2025

No description provided.

xlauko and others added 30 commits August 10, 2025 20:34
This refactors cir floating point type constraints, and fixes long double verifier to use constraints directly.
Apply `CIR_AnyIntOrFloatType` type constraint on element type and rename `elementTy` to `elementType`.
…nion type (llvm#1584)

This PR introduces a new attribute for the explicit byte offset for
`GlobalViewAttr`. It's a proposal, so feel free to reject it once it's
not good from your point of view.

The problem is (as usually) with globals and unions: looks like we can
not really use indexes in the `GlobalView` to address an array of
unions. For example, the next program prints `4` now, but it should be
`42`:
```
typedef struct {
    long s0;
    int  s1;
} S;

typedef union {
   int  f0;
   S f1;
} U;

static U g1[3] = {{42},{42},{42}};
int* g2 = &g1[1].f1.s1;

int main() {       
   (*g2) = 4;
   printf("%d\n", g1[1].f0);    
   return0;
}
``` 
The problem is that we compute wrong indices in
`CIRGenBuilder::computeGlobalViewIndicesFromFlatOffset`. Maybe it can be
even fixed in this case, but I have a feeling that the fix would be a
bit fragile.

So instead of trying to support indexes for the array of unions I
suggest to use the offset explicitly.

From the implementation point of view there are some changes in
`CIRGenBuilder ` - but nothing really new is in there - I just did not
want to introduce copy-pasta for the `isOffsetInUnion` function that is
pretty the same as former `computeGlobalViewIndicesFromFlatOffset`.
Initial implementation  of lowering cir.delete.array llvm#1285 

lowered to a call to **_ZdaPv** keeping similarity with how Clang AST is
lowered.

before 
```LLVM
module @"/opt/tmp/input.cpp" attributes {cir.lang = #cir.lang<cxx>, cir.sob = #cir.signed_overflow_behavior<undefined>, cir.triple = "x86_64-unknown-linux-gnu", cir.type_size_info = #cir.type_size_info<char = 8, int = 32, size_t = 64>, dlti.dl_spec = #dlti.dl_spec<!llvm.ptr<271> = dense<32> : vector<4xi64>, !llvm.ptr<272> = dense<64> : vector<4xi64>, i128 = dense<128> : vector<2xi64>, !llvm.ptr<270> = dense<32> : vector<4xi64>, f80 = dense<128> : vector<2xi64>, i1 = dense<8> : vector<2xi64>, !llvm.ptr = dense<64> : vector<4xi64>, i8 = dense<8> : vector<2xi64>, i16 = dense<16> : vector<2xi64>, i32 = dense<32> : vector<2xi64>, f64 = dense<64> : vector<2xi64>, f128 = dense<128> : vector<2xi64>, f16 = dense<16> : vector<2xi64>, i64 = dense<64> : vector<2xi64>, "dlti.stack_alignment" = 128 : i64, "dlti.endianness" = "little", "dlti.mangling_mode" = "e">} {
  cir.func @_Z17test_delete_arrayPi(%arg0: !cir.ptr<!s32i> loc(fused[#loc3, #loc4])) extra(#fn_attr) {
    %0 = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["ptr", init] {alignment = 8 : i64} loc(#loc8)
    cir.store %arg0, %0 : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>> loc(#loc5)
    %1 = cir.load %0 : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!s32i> loc(#loc6)
    cir.delete.array %1 : <!s32i> loc(#loc6)
    cir.return loc(#loc2)
  } loc(#loc7)
} loc(#loc)
```

after
```LLVM
define dso_local void @_Z17test_delete_arrayPi(ptr %0) #0 {
  %2 = alloca ptr, i64 1, align 8
  store ptr %0, ptr %2, align 8
  %3 = load ptr, ptr %2, align 8
  call void @_ZdaPv(ptr %3)
  ret void
}
```
…llvm#1605)

- Rename `PrimitiveIntOrFPPtr` to `CIR_PtrToIntOrFloatType` and broaden
it to accept any floating-point type.
- Fix incorrect constraint name from `PrimitiveInt` to any `Int`; prior
constraint already allowed arbitrary bitwidths, so the name was
misleading.
- Rename `ComplexPtr` to `CIR_PtrToComplexType` for clarity.
Remove all the code the manages ABI info for arguments and return values
in the initial CIR codegen. We prefer for the CIR to represent the types
as they appear in the source code, with ABI handling being deferred
until the lowering phase or calling convention transform.

The ABI handling being removed here was brought over from the classic
codegen, but none of the effects being computed made it into the CIR so
this change is effectively NFC.

This change leaves the `CIRGenFunctionInfoArgInfo` with just one member.
That can be cleaned up in a later patch.
There are some cases where we're currently using the zero attribute to
initialize global variables for records that aren't properly
zero-initializable. We weren't checking for that, and we also weren't
properly calculating zero-initializability for records. This patch
addresses both of these issues.

This doesn't actually handle the case where the record isn't
zero-initializable. It just reports it as NYI. It does add a comment
about what needs to be done.
Currently, the following code snippet fails with a crash: 
```
#include <stdio.h>

struct X {
  int a;
  X(int a) : a(a) {}
  ~X() {}
};

bool foo(const X &) { return false; }
bool bar() { return foo(1) || foo(2); }
```
it fails with 
```
error: 'cir.yield' op must be the last operation in the parent block
```
The crash can be traced to the [construction of the
TernaryOp](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2728).
A
[yield](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2776)
is created and when the LexicalScope is destroyed the destructors for
the object follow.

So, the CIR looks something like:
```
%11 = "cir.ternary"(%10) ({
    %13 = "cir.const"() <{value = #cir.bool<true> : !cir.bool}> : () -> !cir.bool
    "cir.yield"(%13) : (!cir.bool) -> ()
}, {
    %12 = "cir.const"() <{value = #cir.bool<false> : !cir.bool}> : () -> !cir.bool
    "cir.yield"(%12) : (!cir.bool) -> ()
}) : (!cir.bool) -> !cir.bool
"cir.yield"(%11) : (!cir.bool) -> ()
"cir.call"(%7) <{callee = @_ZN1XD2Ev, calling_conv = 1 : i32, extra_attrs = #cir<extra({nothrow = #cir.nothrow})>, side_effect = 1 : i32}> ({
}) 
```
which is wrong, since the yield should come last. 

The fix here is forcing a cleanup and then the yield follows. A similar
rule also applies
[here](https://github.com/llvm/clangir/blob/5df50096be1a783c26b48ea437523347df8a3110/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp#L2657)
for the "&&" case, e.g., foo(1) && foo(2) instead in the code snippet.

One more modification I made is adding a check for when the current
lexScope is ternary, when creating dispatch blocks for cleanups. I
believe in this case, we do not want to create a dispatch block, please
correct me if I am wrong.

Finally, I add two tests to verify that everything works as intended.
A previous refactoring had reduced the ArgInfo structure to contain a
single member, the argument type. This change eliminates the ArgInfo
structure entirely, instead just storing the argument type directly in
places where ArgInfo had previously been used.
Add mlir Vec support to elementTypeIfVector
This commit un-xfails some tests that were affected by upstream GEP
changes. llvm#1497

The changes are likely introduced by inference of `inbounds` and `nuw`
flags
llvm/llvm-project@10f315d.
It seems to allow LLVM to enable some constant foldings, whose effects
include re-calculating the address using i8 and a single offset. (This
change seems to arise from
https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699/39)
Reapplying
llvm@e66b670
which was reverted during rebase (after fixing some conflicts).
Un-xfails the test `cc1.cir` (llvm#1497).

---------

Co-authored-by: Nathan Lanza <[email protected]>
AmrDeveloper and others added 4 commits September 2, 2025 17:33
This change adds support for CXXDefaultInitExpr for ComplexType
While upstreaming vtt.address_point lowering, I noticed an important
difference in the LLVM IR generated between the CIR path and the OGCG
path. We were incorrectly using i8 as the element type for the GEP
generated when vtt.address_point is lowered with a value argument. This
patch fixes that.
Implement Atomic init for ComplexType
/// If this is a direct call, returns the callee as a `cir::FuncOp` in parent `module`.
/// Otherwise, returns `null`.
/// NOTE: This method walks the symbol table. If you are calling this method a lot,
/// consider using `cir::FuncOp::getDirectCallee(mlir::SymbolTable &)` instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you make such a change? All of the uses in this PR use the other form.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know youre going to query this a lot, you'd run the mlir::SymbolTableAnalysis and use its result for repeated queries.

Copy link
Member

@bcardosolopes bcardosolopes Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not a good use for CIRGen though, given the symbol table is constantly changing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mlir::SymbolTableAnalysis claims to be good at handling insertions and removals. I looked through CIRGen though and couldn't find anywhere that this API could be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andykaylor does that answer your question? Or would you prefer me to remove the symbol table methods until we have a user?

AmrDeveloper and others added 6 commits September 5, 2025 12:11
Implement VisitOpaqueValueExpr for ComplexType
`cir::BreakOp::getBreakTarget` gets the innermost `cir::LoopOpInterface`
or `cir::SwitchOp` containing this `break`.

For example:
```
A: for (...) {
B:   for(...) {
       break; // target = B
C:     switch (...) {
         default: break; // target = C
       }
     break; // target = A
   }     
```

NOTE: This is a part of a broader effort I am working on to make
querying CIR control flow facts more easily. If folks have any design
notes or ideas, please share them.

---------

Co-authored-by: Tommy McMichen <[email protected]>
This implements getting the VTT argument for a delegating constructor
and enables the corresponding test in delegating-ctor.cpp.
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once Andy comments/questions are addressed!

First noted at: llvm#1829
Also:
Adjusted test cases affected by this change.

---------

Co-authored-by: Tommy MᶜMichen <[email protected]>
Copy link
Collaborator

@xlauko xlauko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

bcardosolopes and others added 5 commits September 5, 2025 16:22
This reverts commit a896c41.

Test is failing on Linux (reproduced locally)
Three things:

- Corrected comments to `getZeroInitAttr` as [we return more than only
integrals in that
function](https://github.com/llvm/clangir/blob/2ea4005fa0aa291295b19c200860b5edf9b864b3/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h#L133).
- Given that `emitX86MaskedCompare` and `emitX86MaskedCompareResult`
helpers are pretty large, Added NYI statements on paths not related to
the current set of intrinsics so review is specific to the ones encoded.
- Added test comments related to the behavior observed coming from the
canonicalizer on: llvm#1770
@tommymcm
Copy link
Collaborator Author

tommymcm commented Sep 8, 2025

Rebased after the latest reverts. CI is green

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.