[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

The naming convention used here for COFF is actually very similar to what is 
done for other ThinLTO save-temps output files, which are placed at the input 
module locations. We may want to just do this across the board. @MaskRay wdyt? 
A few other questions/comments below.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104
 
-  auto AddStream = [&](size_t Task) {
+  auto AddStream = [&](size_t Task, Twine File) {
 return std::make_unique(std::move(OS),

I think you might need to mark the new parameter as unused here and in other 
cases where it isn't used via LLVM_ATTRIBUTE_UNUSED, to avoid build warnings - 
I can't recall how strictly that is enforced.



Comment at: lld/COFF/LTO.cpp:229
+StringRef ltoObjName;
+if (bitcodeFilePath == "ld-temp.o") {
+  ltoObjName =

This case should always be i==0 I think?



Comment at: lld/COFF/LTO.cpp:245
+  saveBuffer(buf[i].second, ltoObjName);
 ret.push_back(make(ctx, MemoryBufferRef(objBuf, ltoObjName)));
   }

The above changes affect both the MemoryBufferRef name as well as the saveTemps 
output file name. I assume the change to the former is what is required for 
PDB, is that correct?



Comment at: lld/MachO/LTO.cpp:132
+"ThinLTO", "Thin", config->thinLTOCacheDir,
+[&](size_t task, Twine originFile, std::unique_ptr mb) {
+  files[task] = std::move(mb);

In some files this parameter is called "filed" and in others "originFile". 
Better to be consistent, and perhaps to use a more descriptive name, something 
like moduleName ?



Comment at: llvm/include/llvm/Support/Caching.h:53
 ///
 /// if (AddStreamFn AddStream = Cache(Task, Key))
 ///   ProduceContent(AddStream);

This and possibly other header file descriptions need updates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

@MaskRay wondering if this is a good change to make for ELF as well, wdyt?




Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104
 
-  auto AddStream = [&](size_t Task) {
+  auto AddStream = [&](size_t Task, Twine File) {
 return std::make_unique(std::move(OS),

zequanwu wrote:
> tejohnson wrote:
> > I think you might need to mark the new parameter as unused here and in 
> > other cases where it isn't used via LLVM_ATTRIBUTE_UNUSED, to avoid build 
> > warnings - I can't recall how strictly that is enforced.
> LLVM_ATTRIBUTE_UNUSED is used to suppress unused functions. For unused 
> parameter, I don't see any build warnings when compiling with this patch. I 
> feel like it's very common to have unused parameter. 
LLVM_ATTRIBUTE_UNUSED maps to __attribute__((__unused__)) which works for 
either functions or parameters. However, I see where the macro is defined in 
llvm/include/llvm/Support/Compiler.h that using "(void)unused;" is preferred. 
Either one works (see below). However, it must be the case that there are no 
bots building with -Wunused-parameter, since I see Task is also unused here. So 
nevermind this suggestion, since what you have currently is consistent with 
what is already done here.

```
$ cat unused.cc
int foo(int x1, int x2 __attribute__((__unused__)), int x3) {
  (void)x3;
  return 1;
}
$ clang++ -c unused.cc -Wunused-parameter
unused.cc:1:13: warning: unused parameter 'x1' [-Wunused-parameter]
int foo(int x1, int x2 __attribute__((__unused__)), int x3) {
^
1 warning generated.
```



Comment at: lld/COFF/LTO.cpp:229
+StringRef ltoObjName;
+if (bitcodeFilePath == "ld-temp.o") {
+  ltoObjName =

zequanwu wrote:
> tejohnson wrote:
> > This case should always be i==0 I think?
> IIUC, "ld-temp.o" is the name of combined module. Do you mean there will be 
> only 1 combined module and it will always be the first task?
Yes. So you don't need the handling for i==0 in the name in this case (you 
could probably assert that i==0).



Comment at: llvm/include/llvm/Support/Caching.h:53
 ///
 /// if (AddStreamFn AddStream = Cache(Task, Key))
 ///   ProduceContent(AddStream);

tejohnson wrote:
> This and possibly other header file descriptions need updates.
Can you add a note that ModuleName is the unique module identifier for the 
bitcode module the cache is being checked for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added inline comments.



Comment at: lld/COFF/LTO.cpp:238
+  sys::path::append(path, directory,
+outputFileBaseName + ".lto." + baseName);
+  sys::path::remove_dots(path, true);

MaskRay wrote:
> What if two input bitcode files have the same basename, e.g. `dir1/a.obj` and 
> `dir2/a.obj`?
I think that should be ok as the output file path created here includes the 
directory. So you should get dir1/a.out.lto.a.obj and dir2/a.out.lto.a.obj.



Comment at: lld/COFF/LTO.cpp:229
+StringRef ltoObjName;
+if (bitcodeFilePath == "ld-temp.o") {
+  ltoObjName =

MaskRay wrote:
> MaskRay wrote:
> > tejohnson wrote:
> > > zequanwu wrote:
> > > > tejohnson wrote:
> > > > > This case should always be i==0 I think?
> > > > IIUC, "ld-temp.o" is the name of combined module. Do you mean there 
> > > > will be only 1 combined module and it will always be the first task?
> > > Yes. So you don't need the handling for i==0 in the name in this case 
> > > (you could probably assert that i==0).
> > This looks like a hack. `assert(i == 0)` will fail with 
> > `-opt:lldltopartitions=2`: `buf[1]` will be called `ld-temp.o` as well.
> > 
> > In addition, if an input bitcode file is called `ld-temp.o` (for some 
> > reason they don't use `.obj`, just `ld-temp.o`), `assert(i == 0)` will fail 
> > as well.
> I guess `if (i < config->ltoPartitions)` may fix the issue.
Ah ok, forgot about the lto partitions case. Sorry, @zequanwu, looks like you 
will need to go back to your old handling that appends i if it is non-zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D137217: [LTO][COFF] Use bitcode file names in lto native object file names.

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137217

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D65482: [DAGCombiner] Add an option to control whether or not to enable store merging

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D65482



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were intentionally 
left out due to the LTO concerns mentioned in the description?

Note if we are just passing in the Module and updating the TM based on that, it 
wouldn't hit the threading issue I mentioned in D72245 
, but neither would you get the proper 
aggregation/checking across ThinLTO'ed modules.




Comment at: llvm/include/llvm/Target/TargetMachine.h:188
+  // `M.setTargetTriple(TM->getTargetTriple())` and before
+  // `M.setDataLayout(createDataLayout())`.
+  virtual void resetTargetDefaultOptions(const Module &M) const;

Is there a way to enforce this? Otherwise I foresee fragility. E.g. what if 
this method set a flag on the TM indicating that we have updated it properly 
from the Module, and TargetMachine::createDataLayout() asserted that this flag 
was set?



Comment at: llvm/lib/Target/TargetMachine.cpp:51
+//
+// Override methods should only change DefaultOptions, and use this super
+// method to copy the default options into the current options.


Looks like DefaultOptions is const, so override methods wouldn't be able to 
change it.



Comment at: llvm/lib/Target/TargetMachine.cpp:53
+// method to copy the default options into the current options.
+void TargetMachine::resetTargetDefaultOptions(const Module &M) const {
+  Options = DefaultOptions;

Can you clarify how M will be used - will a follow on patch set the 
MCOptions.ABIName from the Module? Note in the meantime you will likely need to 
mark this with an LLVM_ATTRIBUTE_UNUSED.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

In D72624#1820281 , @lenary wrote:

> In D72624#1817464 , @tejohnson wrote:
>
> >
>
>
> Thank you for your feedback! It has been very helpful.
>
> > I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were 
> > intentionally left out due to the LTO concerns mentioned in the description?
>
> Mostly left out because I wasn't sure how to attack them. I've got an update 
> to this patch which I'm testing right now and it looks much better. I will 
> post that imminently.
>
> > Note if we are just passing in the Module and updating the TM based on 
> > that, it wouldn't hit the threading issue I mentioned in D72245 
> > , but neither would you get the proper 
> > aggregation/checking across ThinLTO'ed modules.
>
> Ok, right, so I think I know what else this patch needs to do. At the moment, 
> I think the `ModFlagBehavior` for module flags are not being checked during 
> ThinLTO. I think this is something that has to be checked for compatibility 
> in `ThinLTOCodeGenerator::addModule` (like the triple is checked for 
> compatibility).


And LTO::addModule (just to add confusion, there are two LTO APIs, 
ThinLTOCodeGenerator is the old one and LTO is the new one, the latter being 
used by lld and the gold plugin).

I had mentioned using LTO::addModule to do the checking in the other patch, but 
there is a complication I should mention:

> I see that the checking behaviour is in `IRMover`, but I don't think ThinLTO 
> uses that, and I don't feel familiar enough with ThinLTO to be sure.

The ThinLTO "link", which is where the modules are added serially, does not 
read IR, only the summaries, which are linked together into a large index used 
to drive ThinLTO whole program analysis. So you can't really read the module 
flags directly during addModule, they need to be propagated via the summary 
flags. The ThinLTO backends which are subsequently fired off in parallel do 
read IR. In those backends, depending on the results of the ThinLTO analysis 
phase, we may use IRMover to link in ("import) functions from other modules. At 
that point, the module flags from any modules that backend is importing from 
will be combined and any errors due to conficting values will be issued.

Thinking through this some more, rather than attempting to fully validate the 
consistency of the module flags across all modules in ThinLTO mode, just rely 
on some checking when we merge subsections of the IR in the ThinLTO backends 
during this importing, which will happen automatically. This is presumably 
where the checking is desirable anyway (in terms of the cases you are most 
interested in catching with ThinLTO, because the IR is getting merged). Note 
that unlike in the full LTO case, where the IR is merged before you create the 
TM, in the ThinLTO case the TM will be created before any of this cross-module 
importing (partial IR merging), so with your patch presumably it will just use 
whatever module flag is on that original Module for it's corresponding ThinLTO 
backend. But since it sounds like any difference in these module flags is an 
error, it will just get flagged a little later but not affect how the TM is set 
up in the correct case. Does that sound reasonable?

> The update to my patch will not address this part of ThinLTO.

I'll take a look through your patch later today or tomorrow, but it may be just 
fine from the above perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

In D72624#1820598 , @dblaikie wrote:

> (just a general comment that this code review should be only in service of 
> the design discussion happening on llvm-dev - please don't approve/commit 
> this without closing out the design discussion there if there are actionable 
> conclusions from this review)


Got it, I will just look at from the LTO perspective. The target ABI specifics 
I haven't followed very closely and are not really my expertise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Teresa Johnson via Phabricator via lldb-commits]

tejohnson added a comment.

From an LTO perspective, this seems fine for the reasons we discussed here. I 
looked through the patch and have a few comments.




Comment at: clang/lib/CodeGen/BackendUtil.cpp:818
+  if (TM) {
+TM->initializeOptionsWithModuleMetadata(*TheModule);
 TheModule->setDataLayout(TM->createDataLayout());

Also needed in EmitAssemblyWithNewPassManager. Maybe it should be moved into 
CreateTargetMachine which would cover both cases.

I'm not sure if this was already discussed - but is there a reason why this 
can't be done in Target::createTargetMachine()? Is it not possible to ensure it 
is called once we have the Module available and pass that in? That would 
centralize this handling and seems cleaner overall.



Comment at: llvm/include/llvm/Target/TargetMachine.h:157
+  const DataLayout createDataLayout() const {
+OptionsCanBeInitalizedFromModule = false;
+return DL;

Do you want to also ensure that createDataLayout is only called iff 
initializeOptionsWithModuleMetadata was previously called? That would need to 
make this a tri-state, or use 2 bools. Then you could assert here that the 
other routine was already called at this point, which would help avoid missing 
calls (like the one I pointed out above), possibly due to future code drift.



Comment at: llvm/include/llvm/Target/TargetMachine.h:192
+  virtual void
+  setTargetOptionsWithModuleMetadata(const Module &M LLVM_ATTRIBUTE_UNUSED) {}
+

Should this be private so that it can only be called via 
initializeOptionsWithModuleMetadata?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits