[PATCH] D146903: (WIP)[SimplifyCFG] add nsw on SwitchToLookupTable

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6126
+                                 /*HasNUW =*/false,
+                                 /*HasNSW =*/!DefaultIsReachable);
+
----------------
Looking at how LinearMultiplier and LinearOffset are generated in SwitchLookupTable, I don't think that always setting NSW is correct. Nothing in the calculation suggests that it can't overflow. I believe we would have to track this separate when these values are computed. I also think that DefaultIsReachable doesn't matter for this part: At this point the range of the input values has already been checked.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6144
+        /*HasNUW =*/false,
+        /*HasNSW =*/!DefaultIsReachable);
 
----------------
I believe in this case both NUW and NSW can always be set. We know that this multiply must be `< BitWidth` to be a valid shift amount, so it can't overflow in either signed or unsigned sense.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6512
     TableIndexOffset = MinCaseVal;
-    TableIndex =
-        Builder.CreateSub(SI->getCondition(), TableIndexOffset, "switch.tableidx");
+    TableIndex = Builder.CreateSub(SI->getCondition(), TableIndexOffset,
+                                   "switch.tableidx", /*HasNUW =*/false,
----------------
Add something like `// If the default is unreachable, all case values are s>= MinCaseVal, so the subtraction cannot wrap in a signed sense.`

This part of the change looks correct to me. It might be worthwhile to split it from the changes to BuildLookup.


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

https://reviews.llvm.org/D146903