Following the discussion on D22038, this refactors a PowerPC specific setcc -> srl(ctlz) transformation so it can be used by other targets.
Details
Diff Detail
Event Timeline
| include/llvm/Target/TargetLowering.h | ||
|---|---|---|
| 3061 | Please add a comment here explaining what this function does, including its relationship to isCtlzFast. | |
| include/llvm/Target/TargetLowering.h | ||
|---|---|---|
| 3061 | A better name might also help: lowerCmpEqZeroToCtlzSrl() ? | |
Just a couple of nits, otherwise LGTM.
But please wait for Hal's feedback on the requested function comment.
| lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
|---|---|---|
| 3566–3567 | Indentation looks wrong - use clang-format. | |
| 3572 | I think it's ok for this patch to be a cut-and-paste and NFC, but a couple of possible improvements for a follow-up:
| |
| lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
|---|---|---|
| 3572 | Sounds good, only I am not sure I understand point 2, are you suggesting to pass the value type as a parameter to this new method? | |
| lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
|---|---|---|
| 3572 | Sorry, I hadn't looked at D23446 when I made this comment. I see that adds a param for 'ExtTy'. I was also wondering what would happen if a target, for example, only had ctlz for a 64-bit type (or it might only have it for a smaller type than i32). I think some more customization would be needed. But to keep it simple, we don't need to deal with any of that in this patch. | |
| include/llvm/Target/TargetLowering.h | ||
|---|---|---|
| 3063 | I think it would be clearer if you include in the comment the form of the transform; something like: // seteq(x, 0) -> truncate(srl(ctlz(zext(x)), log2(#bits))) | |
Please add a comment here explaining what this function does, including its relationship to isCtlzFast.