[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:174
@@ +173,3 @@
+    // getInstSizeInBytes because here all instructions are seen for the first
+    // time.
+    if (InstSize == ~0U)
----------------
rovka wrote:
> I'm not sure I understand this comment. Why wouldn't you check after every use of getInstSizeInBytes?
I wanted to avoid exactly that (checking it after every use) because this function is used quite a lot in this file.
This function computeBlockSize sees all instructions, so actually also checking it elsewhere would be redundant?

================
Comment at: lib/Target/AArch64/AArch64BranchRelaxation.cpp:176
@@ +175,3 @@
+    if (InstSize == ~0U)
+      llvm_unreachable("Failed to get instruction size");
+    Size += InstSize;
----------------
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?


https://reviews.llvm.org/D23355