Skip to content

Conversation

@Bynaryman
Copy link
Contributor

Motivation

Many of CIRCT’s hardware lowering paths (HW/Handshake, HLS) emit scf.index_switch
operations whose cases drive more than one signal (e.g., a mux returning both data
and a valid flag). The existing switch-to-if pass only threaded the first result
through the nested scf.if chain, so multi-result switches either lost data or
crashed during conversion.

Summary of changes

  • Update lib/Transforms/IndexSwitchToIf.cpp so the conversion pattern threads
    the entire result tuple through each nested scf.if (using the adaptor’s
    switch operand when available) and replaces the original switch with every
    result value.
  • Add test/Transforms/switch-to-if.mlir coverage for a switch that yields (i32,f32)
    to ensure every result survives the transformation.

@Bynaryman
Copy link
Contributor Author

@cowardsa could you review it ? thanks.

Copy link
Contributor

@cowardsa cowardsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - pending a small modification/question when handling the getArg return value?

Nice work on the PR otherwise - especially with the nice additional test! Once the changes are submitted tag me and I'll be able to merge for you!

Comment on lines +80 to +82
// CHECK: %[[CDEF:.*]] = arith.constant 30 : i32
// CHECK: %[[FDEF:.*]] = arith.constant 3.000000e+00 : f32
// CHECK: scf.yield %[[CDEF]], %[[FDEF]] : i32, f32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amusing observation - C default -> CDEF (just thought you'd written a string of letters)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realizing that, Constant of the default case in my mind..

Comment on lines 47 to 49
Value switchOperand = adaptor.getArg();
if (!switchOperand)
switchOperand = switchOp.getOperand();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If getArg fails I would have thought this should be a failure? I.e. return failure(); as then the operator is not satisfying its definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I addressed that the simplest possible way @cowardsa

@cowardsa
Copy link
Contributor

Changes look good to me - will merge once actions have run

@cowardsa cowardsa merged commit fa7eb12 into llvm:main Nov 27, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants