search

herd / herdtools7

The Herd toolsuite to deal with .cat memory models (version 7.xx)
Other
215 stars 54 forks source link

Fixes for Neon and SVE #864

Closed murzinv closed 2 months ago

murzinv commented 3 months ago

While looking at issue #860 I noticed few other issues with Neon, so bundled all fixes together.

maranget commented 2 months ago

Hi @murzin. The problem originates in considering that masking symbolic addresses with a 64 bits mask is illegal. The "optimisation" you have removed was hiding this. Allowing such neutral masking looks more robust than hiding the problem under the carpet. I push a fix.

maranget commented 2 months ago

By the way, have you thought about implementing vector values as arbitrary length integers from the Zarith library, as we do for ASL bitfields?

murzinv commented 2 months ago

Hi @murzin. The problem originates in considering that masking symbolic addresses with a 64 bits mask is illegal. The "optimisation" you have removed was hiding this. Allowing such neutral masking looks more robust than hiding the problem under the carpet. I push a fix.

Thanks for having a look into it @maranget ! Please, let me know when fix is available, so I can re-base on top or include it into the series, whatever you prefer :wink:

murzinv commented 2 months ago

By the way, have you thought about implementing vector values as arbitrary length integers from the Zarith library, as we do for ASL bitfields?

Never thought about that to be honest. Primary because 128-bit value already supported by herd and was enough for me, secondary because I was not aware of Zarith library. If there are advantages in moving to Zarith over exiting 128-bit support it would definitely motivate me having a close look :wink:

maranget commented 2 months ago

It's only a suggestion. One advantage would be the ability to have configurable vector lengths.

maranget commented 2 months ago

Thanks for having a look into it @maranget ! Please, let me know when fix is available, so I can re-base on top or include it into the series, whatever you prefer 😉

The quite simple fix is in the last commit I have pushed into your branch. Feel free to adopt or re-write it :)

murzinv commented 2 months ago

Thanks for having a look into it @maranget ! Please, let me know when fix is available, so I can re-base on top or include it into the series, whatever you prefer 😉

The quite simple fix is in the last commit I have pushed into your branch. Feel free to adopt or re-write it :)

Great thanks! I've just put it in front of the series since it is prerequisite for the rest of the fixes.

maranget commented 2 months ago

Hi @murzinv. LGTM. Ready for merge as soon as the typo in test V67 is fixed.

maranget commented 2 months ago

Merged, thanks @murzinv.