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

[MCJIT][test] Move remaining MCJIT interpreter tests to Interpreter/ subdirectory #124744

Conversation

asb
Copy link
Contributor

@asb asb commented Jan 28, 2025

I left these alone in #124463 but I think it makes sense to clean these up as well (which Philip also noted in #124464).


This should be a trivial change, the only reason I held off was wanting to check whether there are any concerns about the tests in the Interpreter/ directory being gated on config.enable_ffi. Some, but not all of the tests being moved contain foreign calls. It seems reasonable to me (even if it's possible a small number of tests could run but don't in some configurations).

…subdirectory

I left these alone in llvm#124463 but I think it makes sense to clean these
up as well (which Philip also noted in llvm#124464).
@asb asb requested review from lhames and preames January 28, 2025 13:41
Copy link

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 606cf887416ba2f136f3104ac12469fc81f73968 c9555cfa503fae0828d9b5a3e3bc803d2d840160 llvm/test/ExecutionEngine/Interpreter/2010-01-15-UndefValue.ll llvm/test/ExecutionEngine/Interpreter/test-interp-target-ext-type.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-cast.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-insertelement.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-insertextractvalue.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-loadstore.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-select.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-shift.ll llvm/test/ExecutionEngine/Interpreter/test-interp-vec-shuffle.ll

The following files introduce new uses of undef:

  • llvm/test/ExecutionEngine/Interpreter/2010-01-15-UndefValue.ll
  • llvm/test/ExecutionEngine/Interpreter/test-interp-vec-insertextractvalue.ll
  • llvm/test/ExecutionEngine/Interpreter/test-interp-vec-shuffle.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

@asb
Copy link
Contributor Author

asb commented Jan 28, 2025

⚠️ undef deprecator found issues in your code. ⚠️

This PR simply moves existing tests, so I think this can reasonably be ignored in the context of this PR (even if in separate work, people might evaluate if undef is desirable or not in these cases).

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

LGTM.

@asb asb merged commit e80d934 into llvm:main Jan 29, 2025
6 of 9 checks passed
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.

3 participants