attach global target features to module-level assembly - #160594
attach global target features to module-level assembly#160594folkertdev wants to merge 1 commit into
Conversation
rustbot
added
the
A-LLVM
rustbot
added
S-waiting-on-review
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| Props.TargetFeatures = StringRef(TargetFeatures, TargetFeaturesLen); | ||
| Props.TargetCPU = StringRef(TargetCPU, TargetCPULen); | ||
| unwrap(M)->appendModuleInlineAsm( | ||
| Module::GlobalAsmFragment(std::string(Asm, AsmLen), std::move(Props))); |
There was a problem hiding this comment.
- the
std::moveis required here. It isn't there in some of the LLVM source, I'm not sure why, but llvm complains loudly without it. - I believe this needs a
std::stringinstead of aStringRefbecauseGlobalAsmFragmentneeds something owned. Hopefully this is right in terms of ownership and cleanup?
| | | ||
| LL | .intel_syntax noprefix | ||
| | ^ | ||
| = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` |
There was a problem hiding this comment.
no idea where this comes from
| llvm::append_module_inline_asm(llmod, &asm, "", ""); | ||
| let asm = create_section_with_flags_asm(".llvmcmd", section_flags, &[]); | ||
| llvm::append_module_inline_asm(llmod, &asm); | ||
| llvm::append_module_inline_asm(llmod, &asm, "", ""); |
There was a problem hiding this comment.
I believe we just embed bytes here, so the target features and cpu don't seem relevant.
This comment has been minimized.
This comment has been minimized.
I don't have the slightest clue what it means to forward target features to assembly.^^ |
|
Fair. So what this does is attach the target features and cpu information to piece of assembly. Previously the target information would sometimes get lost, especially when doing LTO and potentially merging modules with different target features. LLVM would error when assembly used instructions that are gated on a particular target feature, even though the feature is in fact globally enabled. My practical question for you is: am I forwarding the right target features here? and might a particular target cpu that we forward imply target features that we didn't intend to enable? |
9bd2882 to
48835fa
Compare
folkertdev
changed the title
This comment has been minimized.
This comment has been minimized.
|
I have no idea, sorry. I don't even know which potential problems one has to be aware of here. |
48835fa to
fad8008
Compare
This comment has been minimized.
This comment has been minimized.
fad8008 to
e416dec
Compare
|
r? @nikic |
This comment has been minimized.
This comment has been minimized.
e416dec to
9e1ce26
Compare
6 participants
fixes #80608
fixes #127269
At long last, we can forward global target features to LLVM and it will preserve the target features a block of module-level assembly was defined with through LTO.
cc @nikic (who made this happen)
cc @RalfJung any nasty side-effects we might be overlooking here?