[PATCH] D146179: ValueTypes.td: Reorganize ValueType to generate `MachineValueType.h`

[NAKAMURA Takumi via Phabricator via llvm-commits]

chapuni added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:192
 
-  let isAny = true;
+  assert isAny, "iPTRAny should have isOverloaded";
 }
----------------
arsenm wrote:
> I didn't know tablegen had assertions 
I didn't know it was not older functionality. (Introduced in D93911)


================
Comment at: llvm/unittests/Support/MachineValueType.h:80
     /// Return true if this is a FP or a vector FP type.
     bool isFloatingPoint() const {
+      switch (SimpleTy) {
----------------
arsenm wrote:
> Have you measured compile time changes? Seems unfortunate to move from range checks to generated switches 
I over-expected compiler's optimizations easily.

Re. range checks, I have rethought I could rewind them. Emission of ranges (FIRST/LAST_VALUES) was my latest implementation in `VTEmitter`. After it, ranges have become credible values provided by `ValueTypes.td`, since `UpdateVTRange` (D146906) checks also discontinuous gap.

Re. other generated switches, I will measure impact to compile time and I consider emitting range set expressions instead of generated switches.


================
Comment at: llvm/unittests/Support/MachineValueType.h:84
+  case Ty:                                                                     \
+    return (FP ? true : false);
+#include "GenVT.inc"
----------------
kparzysz wrote:
> All of the `x ? true : false` are equivalent to just `x`.
I will fix them. I was afraid of int=>bool conversion for no reason. (Guessing comes from restriction of other language)


================
Comment at: llvm/utils/TableGen/VTEmitter.cpp:78
     auto Value = VT->getValueAsInt("Value");
-
+    bool isInteger = VT->getValueAsInt("isInteger");
+    bool isFP = VT->getValueAsInt("isFP");
----------------
craig.topper wrote:
> Capitalize variable names
Will fix. Excuse my bad habit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146179/new/

https://reviews.llvm.org/D146179