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

Asm blocks should not use indirect output constraints #2349

Open
tyler569 opened this issue Apr 24, 2019 · 3 comments
Open

Asm blocks should not use indirect output constraints #2349

tyler569 opened this issue Apr 24, 2019 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior
Milestone

Comments

@tyler569
Copy link
Contributor

At the moment, asm blocks that do not use the (-> Type) return value mechanism use indirect register constraints for output values. This is a surprising behavior which is causing me to be unable to express the following asm block:

export fn syscall3(number: usize, arg1: usize, arg2: usize, arg3: usize) usize {   
    var result: usize = undefined;                                              
    var is_error: bool = undefined;                                             
                                                                                
    asm volatile (                                                              
        \\ int $0x80                                                            
        : [_] "={rax}" (result), [_] "={@ccc}" (is_error)                        
        : [_] "0" (number),                                                     
          [_] "{rdx}" (arg1),                                                    
          [_] "{rsi}" (arg2),                                                    
          [_] "{rdx}" (arg3)                                                     
    );                                                                          
                                                                                
    if (is_error) {                                                             
        return -result;                                                         
    }                                                                           
    return result;                                                              
}

The ABI in this case is that the error sentinel is passed in the carry flag (CF) in the x86 RFLAGS register ({@ccc}). Zig generates the following LLVM instruction out of this:

call void asm sideeffect " int $$0x80", "=*{rax},=*{@ccc},0,{rdx},{rsi},{rdx}"(i64* %result, i1* %is_error, i64 %6, i64 %7, i64 %8, i64 %9), !db      g !2706

And then fails to allocate the necessary registers to do an indirect load/store into RAX and RFLAGS:

$ zig build-exe test2.zig -target x86_64-freestanding
error: couldn't allocate output register for constraint '{@ccc}'

Clang generates an LLVM instruction with no indirect loads when gives an equivalent function:

  %9 = call { i32, i32 } asm sideeffect "int $$0x80", "={ax},={@ccc},0,{di},~{dirflag},~{fpsr},~{flags}"(i32 %7, i32 %8) #2, !dbg !22, !srcloc !23

Finally, the LLVM language reference discourages using the indirect constraint modifier for register constraints: (taken from https://llvm.org/docs/LangRef.html#indirect-inputs-and-outputs)

It is also possible to use an indirect register constraint, but only on output (e.g. “=*r”). This will cause LLVM to allocate a register for an output value normally, and then, separately emit a store to the address provided as input, after the provided inline asm. (It’s not clear what value this functionality provides, compared to writing the store explicitly after the asm statement, and it can only produce worse code, since it bypasses many optimization passes. I would recommend not using it.)
@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Apr 24, 2019
@andrewrk andrewrk added this to the 0.5.0 milestone Apr 24, 2019
@Sahnvour
Copy link
Contributor

Zig automatically inserts indirect constraint for outputs that are not annotated as return (->), I don't know why nor if it was required at some point.

But before that, your example triggers an assertion in LLVM when verifying the constraints we hand it to.

I think part of the problem is:

  • zig only acknownledges "return types" of an asm expression when using the -> token (see ast_parse_asm_output_item)
  • when doing so, it only allows at most 1 return type from the outputs (see ir_gen_asm_expr)
  • when generating asm via LLVM, your expression states 2 outputs, but the "return type" given by zig is void, since it did not see any -> output: LLVM assert (see LLVMGetInlineAsm in ir_render_asm)

LLVM requires a function type describing the asm expression, where its return type can be void, a type, or a struct containing all the types we noticed as "returns" before. Currently, zig just knows how to handle void or a single type.

I have local changes to make the output types into a struct for the return type of the function, and removed the indirect constraints. Also all references I found online use =@ccc without braces as they are for register names.

Unfortunately there's still an error:

error: couldn't allocate output register for constraint 'c'

@andrewrk andrewrk modified the milestones: 0.5.0, 0.6.0 Sep 27, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.7.0 Jan 5, 2020
@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 30, 2020
@andrewrk andrewrk modified the milestones: 0.8.0, 0.8.1 Jun 4, 2021
@andrewrk andrewrk modified the milestones: 0.8.1, 0.9.1 Sep 1, 2021
@andrewrk andrewrk modified the milestones: 0.9.1, 0.9.0, 0.10.0 Nov 20, 2021
topolarity added a commit to topolarity/zig that referenced this issue Jul 29, 2022
… memory

This change provides a basic implementation of ziglang#2349 for stage2. There's
still quite a lot of work before this logic is as complete as what's in
Clang (https://github.com/llvm/llvm-project/blob/b3645353041818f61e2580635409ddb81ff5a272/clang/lib/CodeGen/CGStmt.cpp#L2304-L2795),
particularly considering the diversity of constraints across targets.

It's probably not worth doing the complete work until there's a clearer
picture for constraints in Zig's future dedicated ASM syntax, but at
least this gives us a small improvement for now.

As a bonus, this also fixes a bug with how we were handling `_`
identifiers.
@topolarity
Copy link
Contributor

LLVM today reports a better error for this: "LLVM ERROR: Flag output operand is of invalid type".

That points us to the actual bug: is_error should be a standard power-of-two integer

The recommendation to pass register outputs directly is still a good one though, since this exposes the store to the optimizer.

topolarity added a commit to topolarity/zig that referenced this issue Jul 29, 2022
… memory

This change provides a basic implementation of ziglang#2349 for stage2. There's
still quite a lot of work before this logic is as complete as what's in
Clang (https://github.com/llvm/llvm-project/blob/b3645353041818f61e2580635409ddb81ff5a272/clang/lib/CodeGen/CGStmt.cpp#L2304-L2795),
particularly considering the diversity of constraints across targets.

It's probably not worth doing the complete work until there's a clearer
picture for constraints in Zig's future dedicated ASM syntax, but at
least this gives us a small improvement for now.

As a bonus, this also fixes a bug with how we were handling `_`
identifiers.
andrewrk pushed a commit that referenced this issue Jul 31, 2022
… memory

This change provides a basic implementation of #2349 for stage2. There's
still quite a lot of work before this logic is as complete as what's in
Clang (https://github.com/llvm/llvm-project/blob/b3645353041818f61e2580635409ddb81ff5a272/clang/lib/CodeGen/CGStmt.cpp#L2304-L2795),
particularly considering the diversity of constraints across targets.

It's probably not worth doing the complete work until there's a clearer
picture for constraints in Zig's future dedicated ASM syntax, but at
least this gives us a small improvement for now.

As a bonus, this also fixes a bug with how we were handling `_`
identifiers.
@andrewrk andrewrk modified the milestones: 0.10.0, 0.10.1 Aug 23, 2022
@topolarity
Copy link
Contributor

This is implemented in stage2 as part of #12282

In stage1, we still use indirect output constraints but the example works when using the right type for is_error

Perhaps this can be closed?

@andrewrk andrewrk modified the milestones: 0.10.1, 0.11.0 Jan 10, 2023
@Vexu Vexu modified the milestones: 0.11.0, 0.12.0 Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

No branches or pull requests

5 participants