[CIR] Un-xfail some tests affected by GEP changes #1621
Conversation
Unmark vtt.cpp and multi-vtable.cpp as XFAIL. Update CIR expects due to 06cee96 (adding `AddressPointAttr`). Update getelementptr expects due to LLVM changes to infer `nuw` llvm/llvm-project@10f315d and subsequent constant foldings performed by LLVM.
Similar to the previous commit. Upstream changes to GEP allows inference of `nuw` and `inbounds` flags, as well as recalculating address offset using i8.
| // LLVM: %[[#V1:]] = alloca ptr, i64 1, align 8 | ||
| // LLVM: %[[#V2:]] = alloca i64, i64 1, align 8 | ||
| // LLVM: store ptr getelementptr (%struct.PackedS2, ptr @g, i64 1), ptr %[[#V1]], align 8 | ||
| // LLVM: store ptr getelementptr inbounds nuw (i8, ptr @g, i64 6), ptr %[[#V1]], align 8 |
There was a problem hiding this comment.
Great, thanks for fixing this! Out of curiosity, did you double check that the LLVM output is the same if you try to codegen the same C sources directly to LLVM without CIR?
There was a problem hiding this comment.
Good question. The LLVM output without CIR seems to not include the constant folding behaviour. Unfortunately I didn't figure out why this diverging behaviour happens.
The LLVM output without CIR seems to sometimes include inrange arguments, which might have prevented the constant folding(?). I'm giving my best guess here.
There was a problem hiding this comment.
It's fine if it differs on the extra annotations (e.g. inrange which we haven't implemented), the important parts are the actual indicies and placement of nuw/nsw. Do these look sound to you?
There was a problem hiding this comment.
I think I'll need another qualified pairs of eyes to have a look --- I'm new to clang/LLVM and still trying to figure things out. The no-clangir path doesn't seem to consistently add nuws, which might be somewhat concerning.
There was a problem hiding this comment.
I'll post compiler explorer links for some examples.
There was a problem hiding this comment.
I think I'll need another qualified pairs of eyes to have a look
no problemo, I can help out.
The no-clangir path doesn't seem to consistently add nuws
This is fine, it's not always needed, we might be over-emitting.
|
|
||
| // LLVM-LABEL: main | ||
| // LLVM: call void @llvm.clear_cache(ptr @buffer, ptr getelementptr (i8, ptr @buffer, i64 32)) | ||
| // LLVM: call void @llvm.clear_cache(ptr @buffer, ptr getelementptr inbounds nuw (i8, ptr @buffer, i64 32)) |
There was a problem hiding this comment.
https://godbolt.org/z/sab5ssce1
Default path has inbounds but not nuw
There was a problem hiding this comment.
Ok, so what I suggest here: please file an issue on the need to fix nuw and point to this PR: the incubator LLVM dialect is a bit behind and hasn't caught up with the GEPNoWrapFlagsProp upstream. So the nuw is probably a side-effect of this mess and let's leave like this until we rebase against upstream.
There was a problem hiding this comment.
(this one also looks good)
| // LLVM: %[[#V1:]] = alloca ptr, i64 1, align 8 | ||
| // LLVM: %[[#V2:]] = alloca i64, i64 1, align 8 | ||
| // LLVM: store ptr getelementptr (%struct.PackedS2, ptr @g, i64 1), ptr %[[#V1]], align 8 | ||
| // LLVM: store ptr getelementptr inbounds nuw (i8, ptr @g, i64 6), ptr %[[#V1]], align 8 |
There was a problem hiding this comment.
I'll post compiler explorer links for some examples.
| // CHECK: cir.global external @packed_element = #cir.global_view<@packed, [-2 : i32, 2 : i32]> | ||
| // LLVM: @packed = global [10 x %struct.PackedStruct] zeroinitializer | ||
| // LLVM: @packed_element = global ptr getelementptr inbounds ([10 x %struct.PackedStruct], ptr @packed, i32 0, i32 -2, i32 2) | ||
| // LLVM: @packed_element = global ptr getelementptr inbounds (i8, ptr @packed, i64 -4) |
There was a problem hiding this comment.
Consistent: https://godbolt.org/z/Ko1TTcjTx
|
I don't have write access to this repo, could you merge my PRs for me please? Thanks! |
yea, been waiting for tests to complete, tks |
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)
2 participants
This commit un-xfails some tests that were affected by upstream GEP changes. #1497
The changes are likely introduced by inference of
inboundsandnuwflags 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)