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

Resolve host CPU to generic outside of x86 #15481

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

bjacob
Copy link
Contributor

@bjacob bjacob commented Nov 8, 2023

Following #15477, cpu is treated as a x86-only thing, except for the special value host that gets resolved to the actual host CPU and CPU features.

JitGlobals creates a host target, that gets resolved accordingly. The resolved CPU is then recorded in a hal.devices.targets attribute. If the CPU field is set to the actual host CPU identifier there, then that will trigger the error message about that being a x86-only thing:

error: Resolution of target CPU to target CPU features is not implemented on this target architecture. Pass explicit CPU features instead of a CPU on this architecture, or implement that.

So this needs to be set to "generic" in that case. No functional difference, as that CPU is just ignored anyway (which is what the error message is about).

@bjacob bjacob marked this pull request as ready for review November 8, 2023 15:33
@bjacob bjacob requested a review from benvanik as a code owner November 8, 2023 15:33
@dcaballe dcaballe requested a review from hcindyl November 8, 2023 17:01
@dcaballe
Copy link
Contributor

dcaballe commented Nov 8, 2023

I wonder if it would make more sense to just emit an error here or at least some kind of warning. It will very surprising/confusing to the user if the pass host with some expectations and we silently change it to generic under the table.

@bjacob
Copy link
Contributor Author

bjacob commented Nov 8, 2023

I wonder if it would make more sense to just emit an error here or at least some kind of warning. It will very surprising/confusing to the user if the pass host with some expectations and we silently change it to generic under the table.

The problem is, because of JitGlobals, the warning would trigger on every run.

Maybe I can change that to keep host unchanged instead of rewriting it to generic. I'll try that.

@hcindyl
Copy link
Contributor

hcindyl commented Nov 8, 2023

Does this PR try to resolve the --iree-llvmcpu-target-cpu usage in RISCV specifically? If so, the flag is actually not used in the llvm RISCV backend, and we don't need the special handling in IREE codebase.

@bjacob
Copy link
Contributor Author

bjacob commented Nov 8, 2023

@dcaballe @benvanik

I have updated this PR with what I feel is the best we can do here. I can't just leave host in the Cpu string, it's just not accepted as a cpu by llvm. So instead I just clear it. Hopefully less confusing.

Again the current state is that ninja iree-test-deps generates lots of errors due to JitGlobals resolving host to an actual Cpu and then that triggering the error about specifying a CPU outside of x86. I dont have an easy solution other than this PR. Just dropping the error would take us back to the previous situation where multiple hours were lost to the confusion caused by the CPU flag doing nothing.

@hcindyl

This is not specifically about RISC-V. I'm running into this on Arm here. Also not explicitly using the flag here. But JitGlobals is causing this to be implicitly used, internally.

@bjacob bjacob merged commit ed32801 into iree-org:main Nov 8, 2023
56 checks passed
@dcaballe
Copy link
Contributor

dcaballe commented Nov 9, 2023

it's just not accepted as a cpu by llvm

If it's so "marginally" used/valuable... why don't we just remove it :)

@bjacob
Copy link
Contributor Author

bjacob commented Nov 9, 2023

If it's so "marginally" used/valuable... why don't we just remove it :)

I'm not saying that cpu=host is not a useful feature to support -- I'm just saying that it isn't something that llvm TargetMachine natively supports. It has to be resolved on our side, like this PR does. Note that outside of x86, while we pass the empty string for cpu, we still resolve host CPU features and pass the corresponding cpuFeatures string. So, host is implemented on all architectures. Just, on our side.

ramiro050 pushed a commit to ramiro050/iree that referenced this pull request Dec 19, 2023
Following iree-org#15477, `cpu` is treated
as a x86-only thing, except for the special value `host` that gets
resolved to the actual host CPU and CPU features.

JitGlobals creates a host target, that gets resolved accordingly. The
resolved CPU is then recorded in a `hal.devices.targets` attribute. If
the CPU field is set to the actual host CPU identifier there, then that
will trigger the error message about that being a x86-only thing:

```
error: Resolution of target CPU to target CPU features is not implemented on this target architecture. Pass explicit CPU features instead of a CPU on this architecture, or implement that.
```

So this needs to be left empty in that case. No functional
difference, as that CPU is just ignored anyway (which is what the error
message is about).
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