Skip to content

Use i1 as result type for llvm.vp.icmp#7183

Closed
dkurt wants to merge 2 commits intohalide:mainfrom
dkurt:fix_vp_intrinsics
Closed

Use i1 as result type for llvm.vp.icmp#7183
dkurt wants to merge 2 commits intohalide:mainfrom
dkurt:fix_vp_intrinsics

Conversation

@dkurt
Copy link
Copy Markdown
Contributor

@dkurt dkurt commented Nov 29, 2022

icmp return type should be i1 but not i32: https://llvm.org/docs/LangRef.html#llvm-vp-icmp-intrinsics

This patch fixed an error which I got on RISC-V:

Intrinsic has incorrect return type!
ptr @llvm.vp.icmp.nxv2i32

@steven-johnson
Copy link
Copy Markdown
Contributor

The buildbots seem to disagree with you, this is failing literally everywhere

@dkurt
Copy link
Copy Markdown
Contributor Author

dkurt commented Nov 30, 2022

@steven-johnson, sorry, forgot that type is still used in the if statement. Fixed that.

@steven-johnson
Copy link
Copy Markdown
Contributor

Looks like it has broken Hexagon/HVX codegen (see https://buildbot.halide-lang.org/master/#/builders/42/builds/549/steps/29/logs/correctness_simd_op_check_hvx):

Failed: vdelta(v*,v*) did not generate for target=hexagon-32-noos-hvx-hvx_128-no_asserts-no_bounds_query-no_runtime vector_width=512. Instead we got:

attn @pranavb-ca for suggestions

@dkurt
Copy link
Copy Markdown
Contributor Author

dkurt commented Nov 30, 2022

Thanks! I've tried debug Hexagon locally but I'm not able to download the SDK.

@steven-johnson
Copy link
Copy Markdown
Contributor

Thanks! I've tried debug Hexagon locally but I'm not able to download the SDK.

Update: this may be unrelated to your PR, I'm getting an apparently-similar failure in main with top-of-tree LLVM. Investigating.

@pranavb-ca
Copy link
Copy Markdown
Contributor

pranavb-ca commented Dec 1, 2022

@aankit-ca - fyi, since I am temporarily working in a timezone ~12 hrs away.

@dkurt dkurt marked this pull request as ready for review December 1, 2022 05:32
@steven-johnson
Copy link
Copy Markdown
Contributor

The "failure" can be ignored, see https://reviews.llvm.org/rG073d5e5945c428e20db0884943e6dcb7ff2158df -- output is different but better, I will update the testcase today. In the meantime, this PR is good to go.

Copy link
Copy Markdown
Contributor

@steven-johnson steven-johnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but adding @abadams as he knows this better than I

@abadams
Copy link
Copy Markdown
Member

abadams commented Dec 1, 2022

Actually I think we want @zvookin

@abadams abadams requested a review from zvookin December 1, 2022 18:16
@dkurt
Copy link
Copy Markdown
Contributor Author

dkurt commented Dec 1, 2022

Is RISC-V instance on buildbot a good option for tests? QEMU can be used to run JIT and AOT tests. I've tried collect some brief proposals here: halide/build_bot#214.

@zvookin
Copy link
Copy Markdown
Member

zvookin commented Dec 1, 2022

I'll try to get to this shortly, but I don't think this is the right thing. (The right thing is for the internals of that function to always use i1 and to cast the result to the requested result type. Also, needs a test, which is on me.)

@dkurt
Copy link
Copy Markdown
Contributor Author

dkurt commented Dec 7, 2022

see #7205

@dkurt dkurt closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants