[CIR][NFC] Rename variables to avoid gcc compilation error #1982
+170,714
−44,667
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Backport creating Array type with ComplexType as element type
As the scf dialect does not support early exits, it might be necessary to change the body of WhileOp to implement the semantics of ContinueOp. I choose to add a guard `if (!cond)` for everything following the `continue`. Co-authored-by: Yue Huang <yue.huang@terapines.com>
This PR is related to llvm#1685 and adds some basic support for the printf function. Limitations: 1. It only works if all variadic params are of basic interger/float type (for more info why memref type operands don't work see llvm#1685) 2. Only works if the format string is definied directly inside the printf function The downside of this PR is also that the handling this edge case adds significant code bloat and reduces readability for the cir.call op lowering (I tried to insert some meanigful comments to improve the readability), but I think its worth to do this so we have some basic printf support (without adding an extra cir operation) until upstream support for variadic functions is added to the func dialect. Also a few more test (which use such a basic form of printf) in the llvm Single Source test suite are working with this PR: before this PR: Testing Time: 4.00s Total Discovered Tests: 1833 Passed : 420 (22.91%) Failed : 10 (0.55%) Executable Missing: 1403 (76.54%) with this PR: Testing Time: 10.29s Total Discovered Tests: 1833 Passed : 458 (24.99%) Failed : 6 (0.33%) Executable Missing: 1369 (74.69%)
This PR addresses the feedback from llvm/llvm-project#142041 (comment). Our algorithm for accumulating bitfields has diverged from CodeGen since Clang 19. There is one key difference: in CIR, we use the function `getBitfieldStorageType`, which checks whether the bit width of the current accumulation run is a valid fundamental width (i.e., a power of two: 8, 16, 32, 64). If it is, it returns a CIR type of that size otherwise, it returns an array with the closest alignment. For example, given the following struct: ```c struct S { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; }; ``` The CodeGen output is: ```llvm %struct.S = type { i64, i16, i32 } ``` Whereas the new CIR algorithm produces: ```mlir !cir.record<struct "S" {!cir.array<!u8i x 7>, !u16i, !u32i}> ``` In CIR, the algorithm accumulates up to field `d`, resulting in 50 accumulated bits. Since 50 is not a fundamental width, the closest alignment is 56 bits, which leads to the type `!cir.array<!u8i x 7>`. The algorithm stops before accumulating field `e` because including it would exceed the register size (64), which is not ideal. At this point, it's unclear whether this divergence from CodeGen represents an improvement. If we wanted to match CodeGen exactly, we would need to replace the use of `getBitfieldStorageType` with `getUIntNType`. The difference is that `getUIntNType` always returns the closest power-of-two integer type instead of falling back to an array when the size is not a fundamental width. With this change, CIR would match CodeGen's layout exactly. It would require the following small code change: ```diff diff --git a/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp index 7c1802b..17538b191738 100644 --- a/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp +++ b/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp @@ -616,7 +616,7 @@ CIRRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, if (!InstallBest) { // Determine if accumulating the just-seen span will create an expensive // access unit or not. - mlir::Type Type = getBitfieldStorageType(astContext.toBits(AccessSize)); + mlir::Type Type = getUIntNType(astContext.toBits(AccessSize)); if (!astContext.getTargetInfo().hasCheapUnalignedBitFieldAccess()) llvm_unreachable("NYI"); @@ -674,12 +674,12 @@ CIRRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field, // remain there after a stable sort. mlir::Type Type; if (BestClipped) { - assert(getSize(getBitfieldStorageType( + assert(getSize(getUIntNType( astContext.toBits(AccessSize))) > AccessSize && "Clipped access need not be clipped"); Type = getByteArrayType(AccessSize); } else { - Type = getBitfieldStorageType(astContext.toBits(AccessSize)); + Type = getUIntNType(astContext.toBits(AccessSize)); assert(getSize(Type) == AccessSize && "Unclipped access must be clipped"); } ``` You can see a comparison between the two functions https://godbolt.org/z/qjx1MaEWG. I'm currently unsure whether using one function over the other has performance implications. Regarding the **ARM error I mentioned in the meeting: it was an `assert` I had forgotten to update. It's now fixed sorry for the confusion.**
- Create CIR specific EnumAttr bases and prefix enum attributes with `CIR_` that automatically puts enum to `cir` namespace - Removes unnecessary enum case definitions - Unifies naming of enum values to use capitals consistently and make enumerations to start from 0 - Remove now unnecessary printers/parsers that gets to be generated automatically
Implement base-2 exponential intrinsic as part of llvm#1192
…lvm#1671) Hi, This is my first here! Tried to mirror some of the patterns already presented in both the codegen lib and its tests I'm very excited to start contributing and potentially making an impact in this project! feedback is much appreciated.
convert from codegen
```c++
assert(!Base.isVirtual() && "should not see vbases here");
auto *BaseRD = Base.getType()->getAsCXXRecordDecl();
Address V = CGF.GetAddressOfDirectBaseInCompleteClass(
Dest.getAddress(), CXXRD, BaseRD,
/*isBaseVirtual*/ false);
AggValueSlot AggSlot = AggValueSlot::forAddr(
V, Qualifiers(),
AggValueSlot::IsDestructed,
AggValueSlot::DoesNotNeedGCBarriers,
AggValueSlot::IsNotAliased,
CGF.getOverlapForBaseInit(CXXRD, BaseRD, Base.isVirtual()));
CGF.EmitAggExpr(InitExprs[curInitIndex++], AggSlot);
if (QualType::DestructionKind dtorKind =
Base.getType().isDestructedType())
CGF.pushDestroyAndDeferDeactivation(dtorKind, V, Base.getType());
```
Moved rd related intrinsic tests, to a different file similar to `clang/test/CodeGen/X86/rd-builtins.c`. Let me know if that's the right call. related: llvm#1404
Update `__real__` operation to use ComplexRealOp and act directly on the complex value. Ref: llvm/llvm-project#144235 (review)
Update `__imag__` operation to use ComplexRealOp and act directly on the complex value. Ref: llvm/llvm-project#144235 (review)
… tzcnt_u64 (llvm#1691) Related: llvm#1404 Implements codegen for the X86 builtins `tzcnt_u16`, `tzcnt_u32`, and `tzcnt_u64`. While adding tests for both the Intel and AMD variants of BMI intrinsics, I ran into issues when placing them in the same file. Both `_tzcnt_u16` (Intel) and `__tzcnt_u16`(AMD) map to the same inline wrapper in <immintrin.h>. Whether they're isolated or both are present in a test file, Clang emits only one definition (`__tzcnt_u16`) which I think causes FileCheck mismatches i.e., the CHECK lines for the Intel version (`test_tzcnt_u16`) would fail when looking for `_tzcnt_u16`. I tried updating the CHECK lines for the Intel test to match the emitted symbol (`__tzcnt_u16`), but it still failed unless the Intel test was run in isolation, and only when CHECK was updated to `_tzcnt_u16` even though `__tzcnt_u16` is what is emitted. I also experimented with split-file to isolate the tests, but that didn’t resolve the issue either. To keep the tests independent, I split the Intel and AMD tests into separate files. Was wondering if this was fine as in OG clang, both Intel and AMD variants are in the same file (https://github.com/llvm/clangir/blob/main/clang/test/CodeGen/X86/bmi-builtins.c)
As we need to preserve the ContinueOp for inner loops when we convert for outer while-loops, we must not mark cir dialect as illegal. Otherwise, MLIR rejects this kind of preservation and considers it as a pass failure. It seems we need another way to check whether the CIR is fully lowered. Co-authored-by: Yue Huang <yue.huang@terapines.com>
Backport ChooseExpr for Scalar expr
Backporting the VecCreateOp Folder from the upstream
Backporting the VecSplatOp simplifier from the upstream
Implement ChooseExpr for ComplexType
Backporting the VecTernaryOp folder
In [libstdc++ std::variant implementation](https://github.com/gcc-mirror/gcc/blob/b0419798447ae25de2f58d1a695db6dadb5d8547/libstdc%2B%2B-v3/include/std/variant#L387-L394), union without any fields is used. According to current CodeGen logic, append 1 byte padding for this kind of union. Handle this union in `mlir::RecordType` for getLargestMember` return nullptr also. The original LLVM IR ```llvm %union.EmptyUnion = type { i8 } @__const._Z2f0v.e = private unnamed_addr constant %union.EmptyUnion undef, align 1 define dso_local void @_Z2f0v() #0 { entry: %e = alloca %union.EmptyUnion, align 1 call void @llvm.memcpy.p0.p0.i64(ptr align 1 %e, ptr align 1 @__const._Z2f0v.e, i64 1, i1 false) ret void } ``` The CIR lowered LLVM IR ```llvm %union.EmptyUnion = type { i8 } define dso_local void @_Z2f0v() #0 { %1 = alloca %union.EmptyUnion, i64 1, align 1 store %union.EmptyUnion undef, ptr %1, align 1 ret void } ``` The major different is original use global const and memcpy, the current use store. The difference between the two is not related to this revision.
Two things: 1. Added some NYI placeholders 2. Tests for i386(x86) are pending as we haven't dealt with that triple yet as compared to CG.
…r calls to memcpy (llvm#1677)
- Generalizes CIRFPTypeInterface files to CIRTypeInterfaces for future type interfaces additions. - Renames CIRFPTypeInterface to FPTypeInterface. - Fixes FPTypeInterface tablegen prefix.
This resolves issues pointed out in https://github.com/llvm/llvm-project/pull/143960/files#r2164047625 of needing to update sized list of types on each new type.
Implement CompoundLiteralExpr for ComplexType
Backport ComplexType support in CallExpr args from the upstream
…ion (llvm#1945) This PR introduces the `cir.indirectbr` operation to support GCC’s labels-as-values extension, which allows `goto` statements to jump to computed block addresses. The implementation creates a dedicated block that hosts the `indirectbr`, where the target address is provided through a PHI Node, similar to how classic code generation handles indirect gotos.
Part of llvm#1912 This PR only fixes 1 file. Also added a CI file to see if it'd run correctly
PR for llvm#1791 --------- Co-authored-by: yotto3s <yupon.tysm@gmail.com>
Backport global initializer for ComplexType from the upstream
Use ZeroInitAttr to initialize a null value for a ComplexType with 0 number of inits, not building ConstComplexType similar to the upstream
closes llvm#1794 This PR adds support for the `__sync_lock_set_and_set` builtin. Signed-off-by: vishruth-thimmaiah <vishruththimmaiah@gmail.com>
Backport the VisitCXXDefaultArgExpr support for ComplexType from the upstream
The builder create methods are deprecated: https://mlir.llvm.org/deprecation/. See https://discourse.llvm.org/t/psa-opty-create-now-with-100-more-tab-complete/87339.
A simple clean up, just some small changes for the use explicitly types instead of `auto`. In the case of `BI__builtin_ia32_pslldqi*_byteshift`. Following the suggestion of this comment: llvm#1886 (comment)
**Related Issue**: llvm#1885
…lvm#1965) The goal of this PR is to prepare the code for backporting the new implementations of Arith operations for ComplexType and other expressions support - Reorder the functions in CIRGenExprComplex similar to upstream and classical codegen. - Remove unnecessary functions `emitAddrOfRealComponent` and `emitAddrOfImagComponent`. - The updated code style is to be similar to upstream.
Use the op create function in the CirGenExprComplex file after the restructure
This backports a problem I noticed while upstreaming constant initialization of automatic aggregate variables. Even though the aggregate is being initialized, we weren't setting the `init` attribute. There is one odd case where the constant being used to initialize a class is an undef. OGCG does the same thing. In this patch I'm not attempting to detect that case, and I'm not sure if we should.
Adds support for the `__sync_swap` builtin. Signed-off-by: vishruth-thimmaiah <vishruththimmaiah@gmail.com>
koparasy
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
andykaylor
approved these changes
Nov 14, 2025
Collaborator
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
Thanks for addressing this! I'm making some suggestions on naming to try to align with classic codegen, but I don't have strong feelings about that.
Comment on lines
113
to
118
| CompilerInstance &CompInstance; | ||
| DiagnosticsEngine &DiagEngine; | ||
| [[maybe_unused]] const HeaderSearchOptions &HdrSearchOptions; | ||
| CodeGenOptions &CGenOptions; | ||
| [[maybe_unused]] const TargetOptions &TgtOptions; | ||
| [[maybe_unused]] const LangOptions &LOptions; |
Collaborator
There was a problem hiding this comment.
Suggested change
| CompilerInstance &CompInstance; | |
| DiagnosticsEngine &DiagEngine; | |
| [[maybe_unused]] const HeaderSearchOptions &HdrSearchOptions; | |
| CodeGenOptions &CGenOptions; | |
| [[maybe_unused]] const TargetOptions &TgtOptions; | |
| [[maybe_unused]] const LangOptions &LOptions; | |
| CompilerInstance &CI; | |
| DiagnosticsEngine &Diags; | |
| [[maybe_unused]] const HeaderSearchOptions &HeaderSearchOpts; | |
| CodeGenOptions &CodeGenOpts; | |
| [[maybe_unused]] const TargetOptions &TargetOpts; | |
| [[maybe_unused]] const LangOptions &LangOpts; |
These changes are suggested to align with the equivalent names in clang/lib/CodeGen/BackendConsumer.h
Contributor
Author
There was a problem hiding this comment.
Happy to follow OG, thank you!
| CIRGenConsumer(CIRGenAction::OutputType Action, | ||
| class CompilerInstance &CompilerInstance, | ||
| class DiagnosticsEngine &DiagnosticsEngine, | ||
| class CompilerInstance &CompInstance, |
Collaborator
There was a problem hiding this comment.
Suggested change
| class CompilerInstance &CompInstance, | |
| class CompilerInstance &CI, |
And likewise, as above.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
30 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The repo no longer builds with gcc:
This fixes these variable naming issues.