Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
The table-lookup strategy seems like overkill to me. As far as I can see, the integer mapping required is: 0→3, 3→2. 2→1, 1→0. In other words, all four input values (that can be handled at all) are just reduced by 1, mod 4. So instead of (147 >> (value << 1)) & 3, you could compute (value - 1) & 3, surely more cheaply.
What about RoundingMode::NearestTiesToAway? There's no support for that in the FPSCR dynamic rounding-mode field, but it looks as if this patch will lower @llvm.set.rounding(i32 4) to some kind of code that silently does the wrong thing.
Perhaps that should have been error-checked by whatever generated the @llvm.set.rounding call in the first place? If so, I think at least a mention in the comments would be useful.
llvm/lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
6131 | Corresponding Arm mode, surely! |
Indeed, it provides more compact code, thank you.
What about RoundingMode::NearestTiesToAway? There's no support for that in the FPSCR dynamic rounding-mode field, but it looks as if this patch will lower @llvm.set.rounding(i32 4) to some kind of code that silently does the wrong thing.
Perhaps that should have been error-checked by whatever generated the @llvm.set.rounding call in the first place? If so, I think at least a mention in the comments would be useful.
Yes, this intrinsic is a low level entity so it is expected that some code elsewhere makes the necessary checks. Appropriate comment is added.
llvm/lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
6131 | Sure) Copy-past error. |
llvm/lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
6148 | Intrinsic IDs should use getTargetConstant I think, but it looks like ARM is already inconsistent on this. Not sure about other targets. |
llvm/lib/Target/ARM/ARMISelLowering.cpp | ||
---|---|---|
6148 | AArch64 is inconsistent as well. |
clang-tidy: warning: invalid case style for function 'LowerSET_ROUNDING' [readability-identifier-naming]
not useful