Ignore hex/decimal differences in 'li' on MIPS #134
Conversation
There was a problem hiding this comment.
Could you change this so this feature can be turned on and off, since this can be a very real diff on N64 (at least on IDO).
See the convo at #134
diff.py
Outdated
| return re.sub( | ||
| f"(-?0x[0-9a-fA-F]+)", lambda m: str(int(m.group(1), 16)), row | ||
| ) |
There was a problem hiding this comment.
Using a compiled regex with re.compile (https://docs.python.org/3/library/re.html#re.compile) would be better
|
I've been playing around with this and only 1 thru 9 appear as 1 thru 9 (rather than 0x1 thru 0x9). Negative values are decimal, and values above 10 are hexidecimal. Does this still need to go behind an option - I know that |
|
Here's a scratch where the difference between the two matters This doesn't match when linked, so asm-differ showing no diffs would be misleading. |
|
btw, for some reason when turning off pseudo instructions asm-differ shows |
|
Wow, I had no idea IDO was this awful 😅 I'll stick it behind an option, one for tomorrow |
|
Have you looked into patching compiler/assembler to avoid this diff instead? Would be nice to avoid option bloat. (Though I'm not really opposed to adding one, either.) |
Guessing this is due to objdump flags. I don't know which ones decomp.me passes |
|
Could be a mips1 vs mips2 thing. The assembler is gnu [edit]: The instruction being assembled is |
|
Setting the arch for mipsel turns the dli into ori: MIPSEL_SETTINGS = replace(MIPS_SETTINGS, name="mipsel", big_endian=False, arch_flags=["-m", "mips:3000"])
|
|
Very open to opinions on the name of the config option, |
It doesn't seem to be a decomp.me issue since it happens locally too |
diff.py
Outdated
| @@ -1575,6 +1576,12 @@ def __init__(self, config: Config) -> None: | |||
| super().__init__(config) | |||
| self.seen_jr_ra = False | |||
|
|
|||
| def _normalize_arch_specific(self, mnemonic: str, row: str) -> str: | |||
| if self.config.ignore_equivalent_immediates and mnemonic == "li": | |||
| regex = re.compile(f",0x([1-9])$") # only 1 thru 9 | |||
There was a problem hiding this comment.
The idea of compiling a regex is to compile it only once, not every time the function is called
There was a problem hiding this comment.
I was googling and read:
Is it worth using Python’s re.compile()?
As you know, Python always internally compiles and caches regexes whenever you use them anyway (including calls to search() or match()), so using compile() method, you’re only changing when the regex gets compiled.
But sure, I'll stick it outside the function
e585446
to
acb381c
Compare
|
I'll need to add this to |
|
OK chaps, please re-review now that I've made some changes; again, if you can think of a better (shorter) name for this option please shout! |
|
This PR will become less important for anyone using |
|
You didn't add a cmdline flag, so this doesn't work as is. Actually, I think it should probably be a project setting, since it feels assembler-/project-specific. Given the commit to maspsx though, is there still a use-case for this? Or should we close it and reopen if the need comes up again? |
|
I'm happy to just close this one, glædelige jul 🎅 |
Is this the best way to tackle this? I put it behind a guard so we aren't trying to do a regex match for every line...
With this change:
Without:
[edit] Just seen the issue here #121, which is pretty much what this PR is trying to address - outside of pseudo operations, its just trying to ignore 0x6 vs 6...