[PATCH] D121968: [AArch64] Fix incorrect getSetCCInverse usage within trySwapVSelectOperands.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked an inline comment as done.
paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17523
+      DAG.getSetCC(SDLoc(SetCC), SetCC.getValueType(), SetCC.getOperand(0),
+                   SetCC.getOperand(1), ISD::getSetCCInverse(CC, NTy));
 
----------------
paulwalker-arm wrote:
> david-arm wrote:
> > Hi @paulwalker-arm, I actually think this is still wrong because I'm pretty sure we should be using the SetCC's input operand type, i.e.
> > 
> >   ISD::getSetCCInverse(CC, SetCC.getOperand(0).getValueType())
> > 
> > The reason is that the setcc may actually use integer operands, i.e.
> > 
> >   %p = icmp eq <vscale x 4 x i32> %c, zeroinitializer
> >   %fmul = fmul <vscale x 4 x float> %a, %b
> >   %sel = select <vscale x 4 x i1> %p, <vscale x 4 x float> %a, <vscale x 4 x float> %fmul
> > 
> > In fact, this is not your fault, but I think we're missing some tests for this. I don't see anything in `trySwapVSelectOperands` that precludes this possibility.
> > 
> Are yes, not sure what I was thinking. Thanks Dave.
Although I've changed one of the tests to use an integer comparison it seems the code wouldn't break anyway because getSetCCInverse is resilient to incorrectly assuming floating point (but not incorrectly assuming integer, which this patch fixes).  This begs the question of why the extra parameter is necessary but that's a story for another day.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121968