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

[Diana Picus via llvm-commits]

rovka 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)
----------------
SjoerdMeijer wrote:
> 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?
Maybe it would now, but that might change someday. I think it would be safer to assert after every use.
Anyway, if you insist on doing this, you should add a similar comment to MSP420BranchSelector, where you're also only checking after the first use.

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


https://reviews.llvm.org/D23355