Skip to content

Conversation

@robbederks
Copy link

Fixing the same bug as #3342 (double FSR increment / decrement when using POSTINC/POSTDEC as both src and dest).

PR #3342 doesn't actually end up writing to the correct registers, since the increment / decrement of FSR already happens before the value is written out again.

Few notes:

  • This might not be the cleanest way to fix this, I'm not too familiar with the syntax. Open to suggestions.
  • There are also a bunch of destREG32 littered around the definitions in a similar fashion, this likely has the same issue but I haven't been able to confirm that.

@GhidorahRex GhidorahRex self-assigned this Sep 10, 2025
@GhidorahRex GhidorahRex added Type: Bug Something isn't working Feature: Processor/PIC Status: Triage Information is being gathered labels Sep 10, 2025
@GhidorahRex
Copy link
Collaborator

Do you have some instruction bytes that demonstrate the problem?

@robbederks
Copy link
Author

Do you have some instruction bytes that demonstrate the problem?

Sure, for example this XORWF.

Before:
before

After:
after

As you can see it fixes FSR0 being incremented twice (which is incorrect)

@GhidorahRex
Copy link
Collaborator

The destREG32 instructions have the same problem. I was able to confirm it with bytesec 52 instruction MOVF PREINC0, f, ACCESS. So that should probably be changed as well.

I think overall the fix is good.

@robbederks
Copy link
Author

The destREG32 instructions have the same problem. I was able to confirm it with bytesec 52 instruction MOVF PREINC0, f, ACCESS. So that should probably be changed as well.

I think overall the fix is good.

Thanks for checking. I'll fix them the same way and ping you once completed (unless you want to split that up in a separate PR).

@GhidorahRex
Copy link
Collaborator

No, it's the same issue, so same PR makes sense.

@robbederks
Copy link
Author

I've looked at the destREG32 instructions more, and the only one that potentially had an issue is the MOVFF instruction (it's the only one with both a srcREG32 and destREG32 defined).

The behaviour with these is actually already correct though, if you specify both differently they have to be individually incremented/decremented, and if they are specified to be the same, it is correct to increment/decrement it twice.

The ec 52 bytes you mentioned are MOVF, which is the non-32 kind, and are fixed already.

Therefor I think this PR is ready to merge.

@GhidorahRex
Copy link
Collaborator

After reviewing this PR, the fix is definitely needed, but this is going about it the wrong way. Because of the way sleigh works, code is going to be duplicated one way or the other, but rather than duplicating the constructor logic code, the fREGLoc sub-constructors should be duplicated and replace the incrementing ones with the non-incrementing versions.

It might be simplest if you rename fREGLoc to srcREGloc and then add destREGloc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature: Processor/PIC Status: Triage Information is being gathered Type: Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants