0

My code should check if a variable has an odd or even number of bits turned on, and then shift right the amount of bits turned on if the amount is even or if it is odd shift left that many times.

This is my code:

IDEAL
MODEL small
STACK 100h
DATASEG
TAV db 00001001b
t_copy db 00001001b

CODESEG
start:

    xor cx,cx
    mov cx,8
    xor al,al
    L1:
        shr [t_copy],1
        jnc nc
        inc al
        nc:
        
        dec cx
        jnz L1
    mov cl,al
    and al,00000001h
    
    cmp al,0h
    jz even_               
    jnz odd
    
    
    odd:
        shl [TAV],cl
        jmp exit
    even_:
        shr [TAV],cl
    
exit:
    mov ax, 4c00h
    int 21h
END start

When I run the code it doesn't shift and doesn't change the value of the variable. I think it changes the value of the variable as index in the memory. Do you know how I can fix it?

Sep Roland
  • 33,889
  • 7
  • 43
  • 76
yahel amity
  • 41
  • 4
  • You didn't initialise `bx`. That's just the most significant error, I don't understand your task entirely so do not know whether you got it right. – ecm Dec 08 '22 at 18:17
  • 1
    What do you mean "not working"? What happens when you run it? What do you see when you single-step with a debugger? [mcve] Or are there errors when you try to assemble? IDK if `mov cl,[byte ptr bx]` is valid syntax; normally `mov cl, byte ptr [bx]`, or since a register implies the destination size, `mov cl, [bx]`. But you haven't set BX at that point in your program, so you're loading nonsense as a shift count. It's not clear what the shift count is supposed to be for the final shift; if that's supposed to be popcount(t_copy). (You don't need a copy in memory; load it into a register.) – Peter Cordes Dec 08 '22 at 18:18
  • 1
    It looks to me like it should work. Inefficient in places, e.g. count into `cl` in the first place (e.g. with the value in another register, [a loop that exits after shifting out the last bit](https://stackoverflow.com/questions/2931497/nasm-count-how-many-bits-in-a-32-bit-number-are-set-to-1)) and `test cl, 1` / `jz even` else fall through (the `jnz odd` is useless, execution already goes there if `jz odd` falls through). If you don't say what it does that's different from what you want, we don't know what kind of bug to look for. – Peter Cordes Dec 08 '22 at 18:42
  • When you single-step with a debugger, is CL=0 when the `shr` runs? If not, it will modify the byte in memory at the `TAV` label. Make sure you're checking it *after* stepping past the shift. Also, you could `mov al, [TAV]` right before the `int 21h`, so you could check the exit status from outside the program. – Peter Cordes Dec 08 '22 at 19:07
  • @PeterCordes i do check the carry flag only after i shift but it's still doesn't work.. it doesn't shift the variable at all.. – yahel amity Dec 08 '22 at 19:17
  • I said CL, not CF. I'm talking about the shift count for the final shift. – Peter Cordes Dec 08 '22 at 19:25
  • I tested it myself (after porting to NASM syntax, since I don't have TASM). It ends up with CL=2 when it runs `shr [TAV], cl`, changing that memory location from 0x9 to 0x2. – Peter Cordes Dec 08 '22 at 19:29

2 Answers2

2

When you run the .EXE that TASM creates for you, execution begins at the start label in the code segment and the CS segment register points at it. For your program to function correctly, similarly the DS segment register should point to the data segment of your program. Sadly this is not so by default since DS is pointing at the Program Segment Prefix PSP. You have to setup DS yourself with the following code:

mov  ax, @data
mov  ds, ax

If you would have chosen for MODEL tiny (instead of MODEL small) the problem would not have shown itself since then all 4 segment registers would have been equal to each other.

xor cx,cx
mov cx,8

Zeroing the register before loading the register is an unnecessary operation.

jnc nc
inc al
nc:

Although this construct is correct, incrementing AL if the carry is set can be much simpler via adc al, 0. If the carry flag is clear nothing is added, and if the carry flag is set then 1 is added.

and al,00000001h
cmp al,0h

Checking whether a value is even/odd is done by looking at the least significant bit which you are doing fine. Point is, you don't need that cmp afterwards since the and instruction already defines the zero flag that you want to use for branching.
Better still, if you used test instead of and, you would receive the same zero flag and not modify the register at all. Writing test cl, 1 would save you from using the additional register.

jz even_
jnz odd

odd:

This jnz conditional jump serves no purpose. If the condition is met the execution jumps to the odd label, and if the condition is not met the execution falls through into the odd label.

shr [t_copy],1

You can improve your solution by processing the data from a register instead of from the memory. You would not need the copy either.

CODESEG
start:
    mov  ax, @data
    mov  ds, ax

    xor  cx, cx     ; CL is popcount, CH is a convenient 0
    mov  al, [TAV]
L1:
    shr  al, 1      ; The bit that comes out of AL
    adc  cl, ch     ;   is added to CL
    test al, al     ; If AL got empty, further adding would be useless
    jnz  L1
    test cl, 1      ; Non-destructive checking of the least significant bit
    jnz  ON
    shr  [TAV], cl  ; Shift right if popcount is EVEN
    jmp  exit               
ON:
    shl  [TAV], cl  ; Shift left if popcount is ODD
exit:
Sep Roland
  • 33,889
  • 7
  • 43
  • 76
0

EDIT: This error described below doesn't actually exist, I misread the code.

You're close, but there's one small mistake here. When you do and al,1, you've actually altered al and the original population count is now lost. Fortunately, there's an easy way to fix this, use test al,1 instead. This affects the zero flag the same way that and al,1 does, except al remains the same it was before the test. So try this out and see if it helps:

    mov cl,al
    test al,1
    
    ;cmp al,0h ;this line is redundant.
    jz even_               
    ;jnz odd   ;this line is redundant.
    odd:
        shl [TAV],cl
        jmp exit
    even_:
        shr [TAV],cl
puppydrum64
  • 1,598
  • 2
  • 15