clangir icon indicating copy to clipboard operation
clangir copied to clipboard

[CIR][LowerToLLVM] Lowered LLVM code for pointer arithmetic should have inbounds

Open liusy58 opened this issue 1 year ago • 8 comments

Fix issue in https://github.com/llvm/clangir/issues/952.

liusy58 avatar Dec 02 '24 03:12 liusy58

Thanks for your time working on this!

The current changes are not related to ClangIR and it's not an appropriate way to resolve #952 . Given the following input code:

void foo(int *iptr) { iptr + 2; }

The CIR generated for the above code would be something similar to:

// ...
%1 = cir.const 2 : i32
%2 = cir.ptr_stride(%0 : !cir.ptr<i32>, %1 : i32), !cir.ptr<i32>
// ...

The generated CIR is further lowered to the following LLVM dialect code in clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp:

%0 = llvm.getelementptr %1[%2] : (!llvm.ptr, i32) -> !llvm.ptr, i32

Apparently the root cause is that LowerToLLVM fails to add the inbounds attribute to the llvm.getelementptr operation. You should update code in LowerToLLVM.cpp accordingly to fix this problem.

Lancern avatar Dec 02 '24 05:12 Lancern

Alright, I'll work on LowerToLLVM to address this issue. In fact, I'm not entirely sure when the inbounds attribute should be added. I examined the code in Value *emitPointerArithmetic from clang/lib/CodeGen/CGExprScalar.cpp, but I couldn't identify a clear pattern. Could you please provide some guidance?

liusy58 avatar Dec 02 '24 06:12 liusy58

@liusy58 The getelementptr instruction is actually emitted in the CodeGenFunction::EmitCheckedInBoundsGEP function defined in the file you pointed out. It has two overloads, and you can find that both overloads set the inbounds flag without many prior conditions.

The C++ standard says that if the result of pointer arithmetic is out of bounds, the behavior is undefined. So I believe for cir.ptr_stride, you should always add the inbounds attribute to the lowered llvm.getelementptr operation.

Lancern avatar Dec 02 '24 09:12 Lancern

Thank you. Let me check it.

liusy58 avatar Dec 02 '24 11:12 liusy58

Thanks @Lancern and @seven-mile for the great review and clarifications. @liusy58 welcome to the ClangIR project!

bcardosolopes avatar Dec 02 '24 23:12 bcardosolopes

@Lancern Hi, I have update the code and could you please review it?

liusy58 avatar Dec 03 '24 09:12 liusy58

Hi, @seven-mile , I have updated the commit, please review it. Thanks!

liusy58 avatar Dec 04 '24 05:12 liusy58

#886 is a related potential area for follow-up work here if you're interested :)

smeenai avatar Dec 05 '24 00:12 smeenai