[PATCH] D66687: [x86] try to form more bt/test + set out of shift+mask patterns

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:5225
+  //       compare in the source type.
+  unsigned ShiftAmt = Srl.getConstantOperandVal(1);
+  unsigned VTBits = VT.getSizeInBits();
----------------
spatel wrote:
> craig.topper wrote:
> > Is it possible for the shift to be on a type larger than 64 bits and have an absurdly out of bounds shift amount that we haven't collapsed to undef yet such that this asserts? Or for that matter a 64 bit shift amount that's larger than 0xffffffff and not been folded yet. Since we truncate a uint64_t to unsigned here.
> Given current limits, I don't think either of those scenarios are possible. IR types are capped well below 2^32:
> http://llvm.org/docs/LangRef.html#integer-type
> 
> And we shouldn't create an overshift node in the 1st place:
> SelectionDAG::simplifyShift() - https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L7120
> 
> If that's somehow violated, then the underlying assert in APInt should fire within SDValue::getConstantOperandVal().
> 
> But there's practically no cost for leaving the shift amount as uint64_t rather than unsigned, so I can change that.
My first scenario was just something like i128 with a shift amount greater than 1 << 64 which would assert in getConstantOperandVal. I know we try to simplify shifts when they're created. I was just worried about a scenario where the shift amount isn't constant when we created it but something folded behind the shift to make it constant and out of bounds then we visited the 'and' node without visiting the shift to simplify it. Its probably not a very realistic scenario, but I know Simon has updated some things to not use getConstantOperandVal because weird scenarios have come up.


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

https://reviews.llvm.org/D66687