[PATCH] D47702: DAG: ComputeNumSignBits from load range metadata
Nicolai Hähnle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 25 00:49:03 PDT 2018
nhaehnle added inline comments.
================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3536-3537
+ ConstantRange CR = getConstantRangeFromMetadata(*Ranges);
+ return std::min(CR.getLower().getNumSignBits(),
+ CR.getUpper().getNumSignBits());
+ }
----------------
First, this should probably use getSignedMin() and getSignedMax() instead of getLower() and getUpper(). An interesting test for this would be a range `{i32 -1, i32 1}`, which to my understanding only allows the values -1 and 0, so should have 32 sign bits, while the code as-is would say that it has 31 sign bits.
The other thing that worries me is what happens when there is a mismatch between the type used in the definition of the range metadata and the type returned by the SDNode. At the very least, there needs to be an assert to check against that case if we think it can't happen. If it //can// happen, it needs to be handled by taking the bit widths of the type of `LD` and of the APInts in the constant range into account.
================
Comment at: test/CodeGen/AMDGPU/load-range-metadata-sign-bits.ll:6
+; GCN-NOT: [[LOAD]]
+define i32 @range_metata_sext_i8_signed_range(i32 addrspace(1)* %ptr) {
+ %val = load i32, i32 addrspace(1)* %ptr, !range !0
----------------
Spelling: *metadata
Also, why can't we always use `v_bfe_i32` for `(ashr (shl X, Y), Y)` (and `v_bfe_u32` for `(lshr (shl X, Y), Y)`), independent of any metadata?
https://reviews.llvm.org/D47702
More information about the llvm-commits
mailing list