This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Override TargetLowering::hasAndNot for Zbb.
ClosedPublic

Authored by craig.topper on Nov 15 2021, 12:52 PM.

Diff Detail

Event Timeline

craig.topper created this revision.Nov 15 2021, 12:52 PM
craig.topper requested review of this revision.Nov 15 2021, 12:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2021, 12:52 PM
Herald added a subscriber: MaskRay. · View Herald Transcript
lebedev.ri resigned from this revision.Nov 15 2021, 12:55 PM

Sadly still completely ignorant about RISC-V, won't have useful insights here.

craig.topper added inline comments.Nov 15 2021, 12:57 PM
llvm/test/CodeGen/RISCV/unfold-masked-merge-scalar-variablemask.ll
1152–1153

This is a size regression, but the comment above it also says this isn't canonical form.

1418–1420

This also appears to be a size regression.

jrtc27 added inline comments.Nov 15 2021, 1:42 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1115–1118

Might possibly make more sense to write as:

if (Subtarget.hasStdExtZbb() && isa<ConstantSDNode>(Y))
  return true;

return false;

depending on how this will be extended in future (e.g. with vector support?)

1118

I think your code size regressions come from this not using Y.getNode()? SDValue is never a ConstantSDNode, so for Zbb this always returns true.

Rebase after fixing DAGCombiner to not cause the size regressions.

Remove TODO that this patch addresses

I'm assuming in_constant_42_vary_invmask is still regressing just because you haven't yet rebased on top of the updated DAGCombine patch and not because something else is missing?

llvm/test/CodeGen/RISCV/unfold-masked-merge-scalar-variablemask.ll
171

Do we still want a TODO for non-Zbb out* to use the shorter sequence that in* use? Currently out* have an extra instruction (two for i64-on-RV32).

craig.topper added inline comments.Nov 15 2021, 6:12 PM
llvm/test/CodeGen/RISCV/unfold-masked-merge-scalar-variablemask.ll
171

I guess that's the RISCV equivalent of https://reviews.llvm.org/D112754

I can add a TODO

jrtc27 added inline comments.Nov 15 2021, 6:14 PM
llvm/test/CodeGen/RISCV/unfold-masked-merge-scalar-variablemask.ll
171

Looks rather familiar... which I guess means it should live in DAGCombine, not X86ISelLowering, though care is needed to avoid circular combines...

jrtc27 accepted this revision.Nov 15 2021, 6:21 PM
This revision is now accepted and ready to land.Nov 15 2021, 6:21 PM
This revision was landed with ongoing or failed builds.Nov 15 2021, 6:48 PM
This revision was automatically updated to reflect the committed changes.