Properly check target_features not to trigger an assertion

[JohnTitor]

Fixes#89875
I think it should be a condition instead of an assertion to check if it's a register as it's possible that reg is a register class.
Also, this isn't related to the issue directly, but is_target_supported doesn't check target_features attributes. Is there any way to check it on rustc_codegen_llvm?

r? @Amanieu

[Amanieu]

The assert is actually correct here, the problem is in the AST lowering pass. It should only allow clobbers for registers that are not supported by the current feature set if these are register clobbers, not register class clobbers.

[JohnTitor]

Hmm, I'm not sure about the right condition then. Since #89641, the target_features check has been moved to rustc_passes, we should add a check there instead of rustc_ast_lowering, right? I'm also not sure how we skip clobbering on rustc_passes (or rustc_ast_lowering).

By the way, the test passes fine on my local and godbolt if I pass the target feature as a compile flag, but CI doesn't, any ideas?

[Amanieu]

OK, so I spent a while looking through the changes in this PR and #89641.

The original assert is still correct and shouldn't be removed. Instead the issue is in the FIXME that you added: if this is fixed to take function-specific target features into account then the assert will never trigger since is_target_supported will return true (which must be the case otherwise it would have been rejected by rustc_passes).

By the way, the test passes fine on my local and godbolt if I pass the target feature as a compile flag, but CI doesn't, any ideas?

CI uses LLVM 10 and the error message seems to come from LLVM. LLVM 10 doesn't support the 't' asm formatting code to print the register name as a ymm register. This is fixed in LLVM 10.0.1 but CI is on 10.0.1. This should be fixed by #90062 which updates the minimum LLVM version to LLVM 11.

[JohnTitor]

Thanks for the help! I think is_target_supported now works properly and it's ready to re-review.

[JohnTitor]

It now passes the tests, @Amanieu could you re-review?

[Amanieu]

@bors r+

[bors]

📌 Commit 12647ea has been approved by Amanieu

[bors]

⌛ Testing commit 12647ea with merge cf1e3d5e2c954b4dc3401f78cbb87d285f642b03...

[bors]

💥 Test timed out

[JohnTitor]

@bors retry

[bors]

⌛ Testing commit 12647ea with merge a9b2bfb...

[bors]

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing a9b2bfb to master...

[rust-timer]

Finished benchmarking commit (a9b2bfb): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression