[PATCH] D88594: [OpenMP] Add Error Handling for Conflicting Pointer Sizes for Target Offload

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3209-3214
+      else if ((T.isArch64Bit() && TT.isArch32Bit()) ||
+               (T.isArch64Bit() && TT.isArch16Bit()) ||
+               (T.isArch32Bit() && TT.isArch64Bit()) ||
+               (T.isArch32Bit() && TT.isArch16Bit()) ||
+               (T.isArch16Bit() && TT.isArch32Bit()) ||
+               (T.isArch16Bit() && TT.isArch64Bit()))
----------------
jhuber6 wrote:
> ABataev wrote:
> > jhuber6 wrote:
> > > ABataev wrote:
> > > > jhuber6 wrote:
> > > > > ABataev wrote:
> > > > > > Maybe do something like `Triple::getArchPointerBitWidth(T.getArch()) != Triple::getArchPointerBitWidth(TT.getArch())"?
> > > > > That was my first thought but that function is private to the Triple class so I just went with this brute force method. The file has this comment for the justification.
> > > > > 
> > > > > 
> > > > > ```
> > > > >    /// Test whether the architecture is 64-bit
> > > > >    ///
> > > > >    /// Note that this tests for 64-bit pointer width, and nothing else. Note
> > > > >    /// that we intentionally expose only three predicates, 64-bit, 32-bit, and
> > > > >    /// 16-bit. The inner details of pointer width for particular architectures
> > > > >    /// is not summed up in the triple, and so only a coarse grained predicate
> > > > >    /// system is provided.
> > > > > ```
> > > > Would it be better to convert the results of `isArch...Bit()` to enum and compare enums then? Like:
> > > > ```
> > > > enum {Arch16bit, Arch32bit, Arch64bit} TArch, TTArch;
> > > > 
> > > > if (T.isArch16Bit())
> > > >   Tarch = Arch16Bit;
> > > > else if ...
> > > > ...
> > > > // Same for TT.
> > > > if (TArch != TTArch)
> > > > ...
> > > > ```
> > > You'd end up with a similar situation right, having about 6 conditionals manually checking check predicate right?
> > > 
> > > ```
> > > if (T.isArch16Bit())
> > > else if (T.isArch32Bit())
> > > else if (T.isArch64Bit())
> > > if (TT.isArch16Bit())
> > > else if (TT.isArch32Bit())
> > > else if (TT.isArch64Bit())
> > > ```
> > Yes, but it may be easier to maintain in the future, say when 128 or 256 bits architectures are added? :)
> > Plus, the last one should not be `else if`, just:
> > ```
> > else {
> >   assert(T.isArch64Bit() && "Expected 64 bits arch");
> >   TArch = ...;
> > }
> > ```
> Something like this sufficient?
> ```
>   enum ArchPtrSize {Arch16Bit, Arch16Bit, Arch16Bit};
>   auto GetArchPtrSize[](llvm::Triple &T) {
>       if (T.isArch16Bit())
>           return Arch16Bit;
>       else if (T.isArch32Bit())
>           return Arch32Bit;
>       else
>           return Arch64Bit;
>   };
> 
>   if (GetArchPtrSize(T) != GetArchPtrSize(TT)) { ... }
> 
> ```
1. Make it `static`.
2. `GetArchPtrSize`->`getArchPtrSize`.
3. No need to use `else` if the substatement is return:
```
if (16bit)
  return Arch16Bit;
if (32Bit)
  return Arch32Bit;
assert(64Bit && "expected 64 bit arch");
return Arch64Bit;
```
4. Move enum to anonymous namespace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88594/new/

https://reviews.llvm.org/D88594