While experimenting with sv.bc in Vertical-First, I discovered an issue where a branch is not take (although the condition for the branch is met). Example test case created in a branch: https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=5a174ba4c2d7c19733167933eee71a2761e38695 Code (With PC/NIA): 00 "setvl 0, 0, %d, 1, 1, 1" % maxvl, # VL = MAXVL = 2, vf=1 04 "sv.cmpi *cr0, 1, *10, 0x10", # compare reg val with immediate 0c "sv.bc 0, *2, 0x10", # jmp if CTR!=0 AND reg not equal to imm 14 "svstep. 27, 1, 0", 18 "bc 4, 3, -0x14", # CR_BI=0, jump to start of loop (sv.cmpi) 1c "or 0, 0, 0", # jump to here if terminate VF loop early Initial value for CTR=MAXVL=2, GPR['10'] = 0x20. Running the simulator and looking at the log output, I can see that - sv.cmpi generates the correct CR0 bits (GPR['10'] > 0x10) Positive (GT) bit set, Zero (EQ) bit cleared - sv.bc: since BO=0, CTR should decrement, and branch if CTR!=0 AND CR_BI=0. CTR is decremented correctly. The branch should be taken as the EQ bit of CR0 is cleared. - NIA should be 0x1c (0x0c + 0x10), the final nop (or 0, 0, 0). BUT instead NIA is set to 0x14 (svstep., the next insn) The 'bc' instruction jumps back to the start of the loop. In this example, the code should never reach svstep, because the processor would branch early. Because of this CTR should remain as 1 (because the second iteration around the loop wouldn't happen). Checking the NIA value using python debugger (pdb) https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/caller.py;h=735252c78f75d54fad9adb522e540c2fdef84ddf;hb=HEAD#l1961 - Breakpoints set in - decoder/isa/generated/svbranch.py, at the start of op_sv_bc() - caller.py: - at call(#l1961) - yield from self.do_nia(asmop, ins_name, rc_en, ffirst_hit) (#l2362) Stepping through the code I got the following: - Running test up to where the auto-generated function op_sv_bc() starts: - CTR is subtracted correctly - ctr_ok true, cond_ok true, test_branch=1, thus NIA will be updated - LR not updated, correct behaviour - Then continuing to run until the yield from self.do_nia - before the yield, the self.namespace['NIA'] is correctly set to 0x1c - Stepping inside self.do_nia: - Not in ffirst mode, thus take the else path - Going into self.check_step_increment(): - self.allow_next_step_inc is false - self.is_svp64_mode is true (make sense) - return value by yielding from self.svstate_post_inc(): - svstate_post_inc() has a default argument of vf=0 - The current SVSTATE vfirst value is compared against the input vf - if (vf=0 and vfirst=1), NIA is updated(). - There are two places in caller.py where svstate_post_inc() is called. In both places, only the required argument 'insn_name' is given, vf remains as 0. This is what causes NIA after sv.bc to be 0x14, instead of 0x1c. (Haven't found relevant bugs to include in 'blocks' or 'see also', adding bug #994 as 'see also' as it is related to addresses.)
(In reply to Andrey Miroshnikov from comment #0) > Example test case created in a branch: > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=5a174ba4c2d7c19733167933eee71a2761e38695 Note I committed this test on a separate branch, so I don't affect master in any way.
(In reply to Andrey Miroshnikov from comment #1) > Note I committed this test on a separate branch, so I don't affect master in > any way. additions are leaf-nodes and have zero impact. think it through: how can the addition of a *new independent* unitvtest have any possible adverse affect on any other code? move it to master.
https://libre-soc.org/openpower/isa/branch/ if AA then NIA <-iea EXTS(LI || 0b00) else NIA <-iea CIA + EXTS(LI || 0b00) if LK then LR <-iea CIA + 4 (In reply to Andrey Miroshnikov from comment #0) > While experimenting with sv.bc in Vertical-First, I discovered an issue > where a branch is not take (although the condition for the branch is met). > > Example test case created in a branch: > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=5a174ba4c2d7c19733167933eee71a2761e38695 > > Code (With PC/NIA): > 00 "setvl 0, 0, %d, 1, 1, 1" % maxvl, # VL = MAXVL = 2, vf=1 > 04 "sv.cmpi *cr0, 1, *10, 0x10", # compare reg val with immediate > 0c "sv.bc 0, *2, 0x10", # jmp if CTR!=0 AND reg not equal to imm > 14 "svstep. 27, 1, 0", this will occur if the jump does NOT take place because the conditions (CTR!=0 AND reg not eq immed) are not met. you can check if the conditions are or are not met by adding *another* nop and increasing the jump from 0x10 to 0x14. if the address jumped to is 0x18 then your hypothesis is correct. if the address of the next ecexuted instruction is still 0x14 then the branch is *not taking place* and the simulator is simply moving to the next instruction. > BUT instead NIA is set to 0x14 (svstep., the next insn) probably because the conditions for *conditional* branching are not met.
(In reply to Luke Kenneth Casson Leighton from comment #3) > this will occur if the jump does NOT take place because the > conditions (CTR!=0 AND reg not eq immed) are not met. I checked it in a debugger on similar assembly, the pseudocode *is* writing to NIA, but then that written value doesn't properly propagate to the fetch of the next instruction.
(In reply to Jacob Lifshay from comment #4) > I checked it in a debugger on similar assembly, the pseudocode *is* writing > to NIA, but then that written value doesn't properly propagate to the fetch > of the next instruction. interesting. there's an update function that is supposed to handle that. wait... that's really obscure because NIA is definitely a variable that is taken care of somewhere around here https://git.libre-soc.org/?p=openpower-isa.git;a=blob;f=src/openpower/decoder/isa/caller.py;hb=HEAD#l2833
(In reply to Luke Kenneth Casson Leighton from comment #2) > move it to master. I rebased and pushed to master: https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=089e6d352ec57be4ab645d18ad9e95df3af0d365
(In reply to Jacob Lifshay from comment #6) > I rebased and pushed to master: > https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff; > h=089e6d352ec57be4ab645d18ad9e95df3af0d365 thx jacob. good find andrey.
I attempted to fix sv.bc, all tests now pass (except the ones that failed before anyway). I pushed to the branch: https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=refs/heads/1210-attempt-to-fix-vf-sv.bc Luke, can you look at the change I made and see if it looks ok? Andrey, please test the attempted fix. https://git.libre-soc.org/?p=openpower-isa.git;a=commitdiff;h=d9544764b1710f3807a9c0685d150a665f70b9a2 commit d9544764b1710f3807a9c0685d150a665f70b9a2 Author: Jacob Lifshay <programmerjake@gmail.com> Date: Mon Nov 20 17:45:23 2023 -0800 fix vertical-first sv.bc https://bugs.libre-soc.org/show_bug.cgi?id=1210
(In reply to Jacob Lifshay from comment #8) > I attempted to fix sv.bc, all tests now pass (except the ones that failed > before anyway). I pushed to the branch: > https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=refs/heads/1210- > attempt-to-fix-vf-sv.bc awesome. > Luke, can you look at the change I made and see if it looks ok? yyyeah it looks like the same "guess" i was considering :) i don't think anything more sophisticated is needed, at this point: if anything else shows up (with extra tests) it can be dealt with. nicely done.
(In reply to Jacob Lifshay from comment #8) > I attempted to fix sv.bc, all tests now pass (except the ones that failed > before anyway). I pushed to the branch: > https://git.libre-soc.org/?p=openpower-isa.git;a=shortlog;h=refs/heads/1210- > attempt-to-fix-vf-sv.bc Thanks for doing that Jacob. > > Andrey, please test the attempted fix. Can confirm the test is now passing (the branch is taken correctly).
pushed to master
fantastic work everybody, that was quite important to have working.