Thanks Andrew. OK, I have added back 'SingleIssue' as syntactic sugar, but not in model, as recommended (if I got you right).
Best Regards, Javed.
What was the bug? How did it manifest?
I somewhat afraid of breaking some existing code. Currently we consider memory around 0 to be "user memory". And in fact it's possible to mmap the first page at least on some linuxes, not sure about other OSes. And there is some effort to port sanitizers to mmu-less environment (https://reviews.llvm.org/D30583). Another possible use if you have N virtual entities and use N as address passed to acquire/release annotations (not perfect, but currently valid).
If we are excluding NULL from user memory, then I think we need to change IsAppMem to return false for NULL. That will make behavior consistent across all functions. Acquire/Release already check IsAppMem for the passed address (at least in debug build). It will also make simpler to undo this decision in some particular contexts.
Thanks for the review!
Closed by commit rL298800
I assume it's ok meanwhile.
LGTM. I think it would be best if Elena or Guy took a look too.
I think this case relevant not only for intrinsic instruction.
Any instruction can be successfully lowered/legalized but the additional instructions that were created can't be legalized and finally when we try to reportGISelFailure, we might be dealing with a erased instruction.
Thanks for taking a look at this, @axw. Is there anything else that needs to happen before this can be merged?
- Fixes by Hal
- Removed the "#include <assert.h>" which caused problems in environments without that header
- Included a directory with -isystem to simulate system headers
- Added a "macro.h" header with definitions of types, functions and macros to simulate the assert.h header and others.
- Added checks for behavior in some situations
It would be even better if we had a fix to the SCEVExpander's pathological behavior.