Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NVPTX] Possible Infinite Loop introduced by [NVPTX] Allow using v4i32 for memcpy lowering. #63294

Closed
justinfargnoli opened this issue Jun 14, 2023 · 4 comments

Comments

@justinfargnoli
Copy link
Contributor

justinfargnoli commented Jun 14, 2023

I believe that I've found an infinite loop introduced by [NVPTX] Allow using v4i32 for memcpy lowering..

Pinging @Artem-B the author of that commit.

The following input causes llc to timeout in Compiler Explorer:

define <5 x i32> @and3_i32v(<5 x i32> %a, <5 x i32> %b) #0 {
  %cmp0 = icmp eq <5 x i32> %a, <i32 1, i32 2, i32 3, i32 4, i32 5>
  %cmp1 = icmp eq <5 x i32> %b, <i32 2, i32 3, i32 4, i32 5, i32 6>
  %sel = select <5 x i1> %cmp1, <5 x i32> %a, <5 x i32> %b
  %sel2 = select <5 x i1> %cmp0, <5 x i32> %sel, <5 x i32> %b
  ret <5 x i32> %sel2
} 

I was also able to observe the infinite loop on my local (modified) fork of LLVM.

This seems to be the code that's causing the infinite loop (link to the source code):

    // We care about the legality of the operation after it has been type
    // legalized.
    while (TLI.getTypeAction(Ctx, VT) != TargetLoweringBase::TypeLegal)
      VT = TLI.getTypeToTransformTo(Ctx, VT);

When debugging the code I saw that getTypeToTransformTo would always set VT to MVT::v4i32 because of "[NVPTX] Allow using v4i32 for memcpy lowering.". Yet, getTypeAction(Ctx, MVT::v4i32) returns TypeSplitVector, not TypeLegal. Thus, causing an infinite loop.

CC @jholewinski NVPTX code owner.

@justinfargnoli
Copy link
Contributor Author

Also, this is my first time submitting a bug to LLVM. Please let me know if there are any conventions that I should be following.

@Artem-B
Copy link
Member

Artem-B commented Jun 14, 2023

Thank you for reporting it.

The comment for the getTypeToTransformTo function explicitly states

/// For types supported by the target, this is an identity function.`

https://github.com/llvm/llvm-project/blob/f9d0bf06319203a8cbb47d89c2f39d2c782f3887/llvm/include/llvm/CodeGen/TargetLowering.h#LL1025C3-L1025C71

I guess the "supported" part is open to interpretation. v4i32 is certainly supported for some operations. But I can see how it will break users implicitly assuming that it would always change the type.

Artem-B added a commit that referenced this issue Jun 14, 2023
The patch may trigger a hang:
#63294

This reverts commit c16b7e5.
@justinfargnoli
Copy link
Contributor Author

Thanks for the prompt response, explanation, and revert @Artem-B.

Closing the issue.

@EugeneZelenko EugeneZelenko added the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label Jun 14, 2023
@Artem-B
Copy link
Member

Artem-B commented Jun 14, 2023

Reverted in eb4f0d9

@EugeneZelenko EugeneZelenko removed the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants