vhscampos marked 2 inline comments as done. vhscampos added inline comments.
================ Comment at: clang/lib/Headers/arm_acle.h:150 +__clsl(unsigned long __t) { +#if __SIZEOF_LONG__ == 4 + return __builtin_arm_cls(__t); ---------------- compnerd wrote: > vhscampos wrote: > > compnerd wrote: > > > I don't see a pattern match for the `cls64` on ARM32, would that not fail > > > to lower? > > Yes. However, for now, I am not enabling support for `cls64` on ARM32 as it > > is not done yet. > Is the difference not just the parameter type? I think that implementing it > should be a trivial change to the existing implementation. Is there a reason > that you are not implementing that? At clang's side, yes, but not in the backend: Arm32 does not have a `cls` instruction, thus the CLS operations need to be custom lowered. In the `llvm.arm.cls(i32)` case, lowering is quite simple, and it's been included in this patch. For `llvm.arm.cls64(i64)`, on the other hand, it is not as trivial since it's necessary to break its logic into 32-bit instructions. So the reason not to implement that (yet) is just to split work in two different efforts. ================ Comment at: clang/lib/Headers/arm_acle.h:155 +#endif +} + ---------------- compnerd wrote: > vhscampos wrote: > > compnerd wrote: > > > Should we have a `__clsll` extension, otherwise these two are the same in > > > LLP64? I'm thinking about the LLP64 environments, where `long` and `long > > > long` are different (32-bit vs 64-bit). > > ACLE does provide a `long long` version of `cls` called `__clsll`. But > > since the support for `cls64` on Arm32 is not done yet, I decided not to > > write support for `__clsll`. If I did, it would work for 64-bit but not for > > 32-bit. > > > > Please let me know what you think. > clang supports Windows where `long` is 4-bytes even on 64-bit targets, and > this means that this doesn't work for that target. I think that we need to > add `__clsll` so that 64-bit ARM at least is covered. I'm not sure if I am following you. On AArch64-Windows, `__clsl` will be lowered to `llvm.aarch64.cls(i32)` which will then be custom lowered correctly. Let me know if I am thinking this wrong. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69250/new/ https://reviews.llvm.org/D69250 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits