Shift right/left based on an even/odd popcount-CodePudding

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?

CodePudding user response：

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:

CodePudding user response：

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