[PATCH] D102969: [GlobalISel] Add G_SBFX/G_UBFX to computeKnownBits

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:522
+      Known = KnownBits::ashr(KnownBits::shl(Known, ShiftKnown), ShiftKnown);
+    }
+    break;
----------------
Would it be clearer to handle G_SBFX/G_UBFX separately?


================
Comment at: llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:514
+        WidthKnown.getMinValue().getLimitedValue(BitWidth));
+    Known = KnownBits::lshr(SrcOpKnown, OffsetKnown) & Mask;
+    if (Opcode == TargetOpcode::G_SBFX) {
----------------
bcahoon wrote:
> foad wrote:
> > RKSimon wrote:
> > > foad wrote:
> > > > RKSimon wrote:
> > > > > Are you going to have any issues here when Mask.hasConflict() is true?
> > > > That can only happen if WidthKnown.getMinValue() > WidthKnown.getMaxValue(), which I guess can only happen if WidthKnown.hasConflict() was already true. What are the rules about introducing or propagating conflicts?
> > > Well, we do assert there is no conflict at the end of GISelKnownBits::computeKnownBitsImpl so we should be ensuring this doesn't happen.
> > Right, but I'm saying that this code can't produce a conflict out of thin air, only if one of the inputs already had a conflict, so I *think* it's OK. Would you agree?
> I'm not sure what, if anything, needs to be done to handle a conflict here? Appreciate any suggestions.
Yes. I think you're right.


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

https://reviews.llvm.org/D102969