compnerd 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);
----------------
vhscampos wrote:
> 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.
Would it not be sufficient to do the top half (after a shift right of 32-bits),
and if it is exactly 32, then do the bottom 32-bits, otherwise, you're done?
================
Comment at: clang/lib/Headers/arm_acle.h:155
+#endif
+}
+
----------------
vhscampos wrote:
> 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.
Windows is LLP64, `__cls` and `__clsl` are the same thing. It needs a
`__clsll` for the 64-bit value.
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
