[PATCH] D23355: getInstSizeInBytes: sentinel value fix for AArch64, AMDGPU, MSP430

[Sjoerd Meijer via llvm-commits]

SjoerdMeijer added inline comments.

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:176
@@ +175,3 @@
+    if (InstSize == ~0U)
+      llvm_unreachable("Failed to get instruction size");
+    Size += InstSize;
----------------
rovka wrote:
> SjoerdMeijer wrote:
> > rovka wrote:
> > > I think you can use an assert instead (here and everywhere else).
> > I wanted to make this a non-functional change. The current/old behaviour is that we can run into a compiler abort (unreachable) for unsupported instructions. If we change unreachable into asserts, then the new behaviour for Release builds that don't have asserts is different. I.e., instead of aborting, the compiler continues its analysis with wrong numbers (it would wrap around as we add ~0U). I think that is undesired behaviour, would you agree?
> I'm not 100% sure about that.
> In the coding standards it says: "When assertions are disabled (i.e. in release builds), llvm_unreachable becomes a hint to compilers to skip generating code for this branch. If the compiler does not support this, it will fall back to the “abort” implementation."
> So, yes, we see aborts now, but I'm not convinced that's always the intention (OTOH that might be old text, or I might be misreading it).
Alright, excellent, I am actually more than happy to use asserts because it is much cleaner! And then I will simply do it after each call.


https://reviews.llvm.org/D23355