[PATCH] D135302: [AArch64] Support SETCCCARRY lowering

Sat Oct 8 03:10:29 PDT 2022

fzhinkin added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:417
   setOperationAction(ISD::JumpTable, MVT::i64, Custom);
+  setOperationAction(ISD::SETCCCARRY, MVT::i32, Custom);
+  setOperationAction(ISD::SETCCCARRY, MVT::i64, Custom);
----------------
fzhinkin wrote:
> efriedma wrote:
> > fzhinkin wrote:
> > > efriedma wrote:
> > > > fzhinkin wrote:
> > > > > efriedma wrote:
> > > > > > Do we ever actually use SETCCCARRY for i32 on targets where i64 is legal?
> > > > > `SelectionDAGLegalize::LegalizeOp` uses default case to handle SETCCCARRY and calls `TLI.getOperationAction`  with SETCCCARRY's type (which is `i32`). I guess that this question never arose previously because for other targets there were either no 64-bit registers (ARM), or because various value types were supported (X86, M86k). 
> > > > > 
> > > > > Not sure if it should be fixed along with this change or via separate commit.
> > > > Oh, I see what you mean.  The current way is fine; just wasn't really thinking about the output type.
> > > I'm not sure that it supposed to work this way, because while `SelectionDAGLegalize::LegalizeOp` looks at SETCCCARRY's output type, `DAGTypeLegalizer::IntegerExpandSetCCOperands` uses operand's type.
> > Oh, hmm.  They should use the same type; that's probably worth fixing.  (I don't think it causes a practical problem here, but it could in other cases.)
> Sure, I'll fix it first then, thanks!
Added fix to this change as there were no other way to test its correctness.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135302