[CIR][CIRGen][builtin] handle `__lzcnt` #1382

FantasqueX · 2025-02-21T17:01:26Z

Traditional clang implementation:

Lines 3618 to 3632 in a1ab6bf

    
           case Builtin::BI__lzcnt16: 
        
           case Builtin::BI__lzcnt: 
        
           case Builtin::BI__lzcnt64: { 
        
             Value *ArgValue = EmitScalarExpr(E->getArg(0)); 
        
             llvm::Type *ArgType = ArgValue->getType(); 
        
             Function *F = CGM.getIntrinsic(Intrinsic::ctlz, ArgType); 
        
             llvm::Type *ResultType = ConvertType(E->getType()); 
        
             Value *Result = Builder.CreateCall(F, {ArgValue, Builder.getFalse()}); 
        
             if (Result->getType() != ResultType) 
        
               Result = Builder.CreateIntCast(Result, ResultType, /*isSigned*/true, 
        
                                              "cast"); 
        
             return RValue::get(Result); 
        
           }

The problem here is that __builtin_clz allows undefined result, while __lzcnt doesn't. As a result, I have to create a new CIR for __lzcnt. Since the return type of those two builtin differs, I decided to change return type of current CIR_BitOp to allow new CIR_LzcntOp to inherit from it.

I would like to hear your suggestions. C.c. @Lancern

Lancern · 2025-02-22T12:35:02Z

The problem here is that __builtin_clz allows undefined result, while __lzcnt doesn't. As a result, I have to create a new CIR for __lzcnt.

Have you considered updating the existing cir.bit.clz operation and add a unit attribute that specifies whether an input value of 0 is poisoned?

bcardosolopes · 2025-02-27T01:54:24Z

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

@@ -1714,6 +1714,30 @@ def BitPopcountOp
  }];
 }

+def BitLzcntOp : CIR_BitOp<"bit.lzcnt", AnyTypeOf<[UInt16, UInt32, UInt64]>> {


This can share a tablegen class with BitPopcountOp.

Why BitPopcount? Do you mean BitClzOp? The reason is their different poison bit.

I was actually curious if you could reuse the CIR_BitOp if you make it a bit more generic.

github-actions · 2025-03-01T14:21:19Z

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

FantasqueX · 2025-03-01T14:25:05Z

The new LzcntOp doesn't inherent BitOp. So, I can avoid touching original code.

Lancern

I'm still unclear about why we don't just update cir.bit.clz for __lzcnt. AFAIK, we may have two problems in doing so:

The result type of cir.bit.clz is fixed to be !s32i. Have you tried update the TableGen definition of cir.bit.clz so you could relax this constraint?
cir.bit.clz would invoke undefined behaviors if the operand is 0. To overcome this, we could add a UnitAttr to the parameters of cir.bit.clz that indicates whether the operation should poison an input value of 0. Have you tried this?

Are there any other concerns I'm missing here?

Lancern · 2025-03-01T17:17:19Z

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

+    Compute the number of leading 0-bits in the input.
+
+    The input integer must be an unsigned integer. The `cir.bit.lzcnt` operation
+    returns the number of consecutive 0-bits at the most significant bit
+    position in the input.
+
+    If the input value is 0, the return value is the size of the input value.


Add some more description about the differences between the cir.bit.clz operation.

Yea, that'd be great

bcardosolopes · 2025-03-04T03:26:31Z

+1 on all comments from @Lancern

FantasqueX · 2025-03-04T19:53:47Z

@Lancern @bcardosolopes Thanks for your review. I'm going to make some changes. First, changing the return type of CIR_BitOp to match input. Second, adding a new base class for CIR_ClzOp and CIR_CtzOp which contains a unit attribute to indicate poison. For future work, specifically handling BI__builtin_clzg, we can add more logic in CodeGen stage and reuse CIR_ClzOp. I would like to hear your suggestions❤️

FantasqueX · 2025-03-06T07:29:56Z

I have reworked the patch. Creating a base class for ClzOp and CtzOp which contains an I1Attr just like MLIR LLVMIR. And change BitOp to require same operand and result type. So, at CodeGen stage, clz will emit a ClzOp and a CastOp. Also changing lowering stage to emit llvm.intr.ctlz directly install of calling intrinsics. This patch also fixes an IR difference for ffs like op. Before, CIR casts after ctlz and uses sint32 to calculate. However, OG calculates first and then casts. This patch removes ThroughMLIR lowering for ClzOp and CtzOp if is_zero_poison is true.

FantasqueX · 2025-03-06T07:31:54Z

I also noticed that MLIR LLVMIR doesn't require SameOperandsAndResultType for llvm.intr.ctlz, llvm.intr.cttz and llvm.intr.ctpop.

Lancern

Another round of review! Some comments inline.

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

clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp

clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp

Lancern

Overall looks good to me, some nits inline.

clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp

FantasqueX · 2025-03-13T12:52:10Z

I also noticed that MLIR LLVMIR doesn't require SameOperandsAndResultType for llvm.intr.ctlz, llvm.intr.cttz and llvm.intr.ctpop.

Created a PR to resolve this llvm/llvm-project#131133

@Lancern

Traditional clang implementation: https://github.com/llvm/clangir/blob/a1ab6bf6cd3b83d0982c16f29e8c98958f69c024/clang/lib/CodeGen/CGBuiltin.cpp#L3618-L3632 The problem here is that `__builtin_clz` allows undefined result, while `__lzcnt` doesn't. As a result, I have to create a new CIR for `__lzcnt`. Since the return type of those two builtin differs, I decided to change return type of current `CIR_BitOp` to allow new `CIR_LzcntOp` to inherit from it. I would like to hear your suggestions. C.c. @Lancern

FantasqueX requested review from lanza and bcardosolopes as code owners February 21, 2025 17:01

FantasqueX marked this pull request as draft February 23, 2025 09:58

FantasqueX mentioned this pull request Feb 23, 2025

[RFC] Refine design of CIR_BitOp #1392

Closed

bcardosolopes reviewed Feb 27, 2025

View reviewed changes

FantasqueX force-pushed the builtin-lzcnt branch from 0e571e7 to 17557ab Compare March 1, 2025 14:14

FantasqueX marked this pull request as ready for review March 1, 2025 14:17

FantasqueX requested a review from Lancern March 1, 2025 14:18

Lancern reviewed Mar 1, 2025

View reviewed changes

FantasqueX force-pushed the builtin-lzcnt branch from 2d8eca3 to 0fbbce2 Compare March 6, 2025 07:23

Lancern requested changes Mar 6, 2025

View reviewed changes

FantasqueX force-pushed the builtin-lzcnt branch from 0fbbce2 to 445b348 Compare March 7, 2025 07:59

FantasqueX added 11 commits March 12, 2025 21:03

[CIR][CIRGen][builtin] handle __lzcnt

48220c4

format code

c29bafc

fix bit.cir test

22b2251

Redesign LzcntOp

f25f998

Restore last line

e21f2e4

Restore last line

5e00cc1

format code

7367661

Redine CIR clz

7739cae

use mlir::Value instead of auto

35d311e

use raw bool

e360f43

Emit an error message

65ce1d9

use unitattr instead of i1attr

ef47a3c

FantasqueX force-pushed the builtin-lzcnt branch from b29e7d7 to ef47a3c Compare March 12, 2025 13:24

FantasqueX requested a review from Lancern March 12, 2025 13:24

add doc of zero_poison

8799459

Lancern approved these changes Mar 12, 2025

View reviewed changes

clang/lib/CIR/Lowering/ThroughMLIR/LowerCIRToMLIR.cpp Outdated Show resolved Hide resolved

FantasqueX added 2 commits March 12, 2025 23:42

ignore zero_poison when lowering through MLIR

0a6485b

remove unintended line

c43e956

FantasqueX requested a review from bcardosolopes March 12, 2025 15:45

bcardosolopes merged commit 6f03ca6 into llvm:main Mar 12, 2025
6 checks passed

FantasqueX deleted the builtin-lzcnt branch March 12, 2025 18:27

	case Builtin::BI__lzcnt16:
	case Builtin::BI__lzcnt:
	case Builtin::BI__lzcnt64: {
	Value *ArgValue = EmitScalarExpr(E->getArg(0));

	llvm::Type *ArgType = ArgValue->getType();
	Function *F = CGM.getIntrinsic(Intrinsic::ctlz, ArgType);

	llvm::Type *ResultType = ConvertType(E->getType());
	Value *Result = Builder.CreateCall(F, {ArgValue, Builder.getFalse()});
	if (Result->getType() != ResultType)
	Result = Builder.CreateIntCast(Result, ResultType, /isSigned/true,
	"cast");
	return RValue::get(Result);
	}

[CIR][CIRGen][builtin] handle __lzcnt #1382

[CIR][CIRGen][builtin] handle __lzcnt #1382

Uh oh!

Conversation

FantasqueX commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lancern commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcardosolopes Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

FantasqueX Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

bcardosolopes Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

FantasqueX commented Mar 1, 2025

Uh oh!

Lancern left a comment • 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!

Lancern Mar 1, 2025

Choose a reason for hiding this comment

Uh oh!

bcardosolopes Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

bcardosolopes commented Mar 4, 2025

Uh oh!

FantasqueX commented Mar 4, 2025

Uh oh!

FantasqueX commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FantasqueX commented Mar 6, 2025

Uh oh!

Lancern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lancern left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FantasqueX commented Mar 13, 2025

Uh oh!

Uh oh!

[CIR][CIRGen][builtin] handle `__lzcnt` #1382

[CIR][CIRGen][builtin] handle `__lzcnt` #1382

FantasqueX commented Feb 21, 2025 •

edited

Loading

Lancern commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Mar 1, 2025 •

edited

Loading

Lancern left a comment •

edited

Loading

FantasqueX commented Mar 6, 2025 •

edited

Loading