In the context of atomic load, integer, pointer and float point types are allowed, thus we should allow llvm.ppc.cfence to accept any type mentioned.
Details
- Reviewers
nemanjai shchenz vchuravy - Group Reviewers
Restricted Project - Commits
- rG6710b21d4698: [PowerPC] Allow llvm.ppc.cfence to accept pointer types
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Should be safe to LLVM-14, if there is 14.0.6. And noted that, when it's landed, it can be cherry-picked cleanly to LLVM-14 due to the floating point tests added.
LGTM with one nit.
Thanks for fixing this.
llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
---|---|---|
1500 | Maybe we need to add assertion in PPCTargetLowering::emitTrailingFence to explicitly check when we generate ppc_cfence, we are not generating fence for any undesired types? After this change, any type is accepted, for example fp128 or vector types. IR level verifier can be disabled and the assertion when lower this intrinsic in ISEL is a little late IMO. |
Looks like this breaks tests on non-linux: http://45.33.8.238/macm1/37997/step_11.txt , http://45.33.8.238/win/60800/step_11.txt
Please take a look and revert for now if it takes a while to fix.
Hi, I've pushed a fix https://github.com/llvm/llvm-project/commit/106657df4cb36b325056efc57c91340d3d85236d. Should be green in next build.
llvm/test/CodeGen/PowerPC/cfence-double.ll | ||
---|---|---|
7 ↗ | (On Diff #439609) | Is the plan here to do something better than assert? Perhaps emit a fatal error? What happens on non-assert builds on these tests now? Something terrible? |
llvm/test/CodeGen/PowerPC/cfence-double.ll | ||
---|---|---|
7 ↗ | (On Diff #439609) | I've posted https://reviews.llvm.org/D127563 to fix cfence on floating point types on PowerPC side. Alternative solution would be https://reviews.llvm.org/D127609(since the solution on PowerPC side assumes the floating point value is converted from integer value). For non-assert builds, the test causes an ICE. |
llvm/test/CodeGen/PowerPC/cfence-double.ll | ||
---|---|---|
7 ↗ | (On Diff #439609) | The ICE is something like LLVM ERROR: Cannot select: 0x6534da8: i64 = any_extend 0x6534b38 0x6534b38: f64 = bitcast 0x6534ad0 0x6534ad0: i64,ch = AtomicLoad<(load monotonic (s64) from %ir.0)> 0x64befe8, 0x6534a68 0x6534a68: i64,ch = CopyFromReg 0x64befe8, Register:i64 %0 0x6534a00: i64 = Register %0 |
Maybe we need to add assertion in PPCTargetLowering::emitTrailingFence to explicitly check when we generate ppc_cfence, we are not generating fence for any undesired types? After this change, any type is accepted, for example fp128 or vector types.
IR level verifier can be disabled and the assertion when lower this intrinsic in ISEL is a little late IMO.